Skip to content

Conversation

@HeartSaVioR
Copy link
Contributor

What changes were proposed in this pull request?

This patch proposes to change the log level of logging generated code in case of compile error being occurred in UT. This would help to investigate compilation issue of generated code easier, as currently we got exception message of line number but there's no generated code being logged actually (as in most cases of UT the threshold of log level is at least WARN).

Why are the changes needed?

This would help investigating issue on compilation error for generated code in UT.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

N/A

@HeartSaVioR HeartSaVioR changed the title [MINOR][SQL] Always log generated code in case of compile error on generated code in UT [MINOR][SQL] Set log level of log generated code as ERROR in case of compile error on generated code in UT Sep 18, 2019
@maropu
Copy link
Member

maropu commented Sep 19, 2019

plz file a jira for better traceability?

@maropu maropu changed the title [MINOR][SQL] Set log level of log generated code as ERROR in case of compile error on generated code in UT [SPARK-XXXXX][SQL][TEST] Set log level of log generated code as ERROR in case of compile error on generated code in UT Sep 19, 2019
@HeartSaVioR
Copy link
Contributor Author

OK I'll file one. Thanks for the guide.

@maropu
Copy link
Member

maropu commented Sep 19, 2019

Thanks!

private def logGeneratedCode(code: CodeAndComment): Unit = {
val maxLines = SQLConf.get.loggingMaxLinesForCodegen
if (Utils.isTesting) {
// log as ERROR so that it can be always logged in UT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since its clear what the code is, I personally think we don't need this comment.

@maropu
Copy link
Member

maropu commented Sep 19, 2019

cc: @cloud-fan @viirya

@HeartSaVioR HeartSaVioR changed the title [SPARK-XXXXX][SQL][TEST] Set log level of log generated code as ERROR in case of compile error on generated code in UT [SPARK-29165][SQL][TEST] Set log level of log generated code as ERROR in case of compile error on generated code in UT Sep 19, 2019
@cloud-fan
Copy link
Contributor

@dongjoon-hyun do you know how to re-trigger the github checks? One of them fails and it seems flaky.

@SparkQA
Copy link

SparkQA commented Sep 19, 2019

Test build #110940 has finished for PR 25835 at commit b78d0e0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 19, 2019

Test build #110953 has finished for PR 25835 at commit d836138.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

@cloud-fan . Yes. I knew it and retriggered before in another PRs, but the current UI seems to have a bug. In general, click the Details. Then, Recheck all ... button should be there in the same position where Cancel check suite exists. It looks like GitHub Action bug. I'll try to report it GitHub Action community.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. This is very useful. Merged to master/2.4.
Thank you, @HeartSaVioR , @maropu , @cloud-fan , @viirya .

dongjoon-hyun pushed a commit that referenced this pull request Sep 19, 2019
… in case of compile error on generated code in UT

### What changes were proposed in this pull request?

This patch proposes to change the log level of logging generated code in case of compile error being occurred in UT. This would help to investigate compilation issue of generated code easier, as currently we got exception message of line number but there's no generated code being logged actually (as in most cases of UT the threshold of log level is at least WARN).

### Why are the changes needed?

This would help investigating issue on compilation error for generated code in UT.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

N/A

Closes #25835 from HeartSaVioR/MINOR-always-log-generated-code-on-fail-to-compile-in-unit-testing.

Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit eee2e02)
Signed-off-by: Dongjoon Hyun <[email protected]>
@dongjoon-hyun
Copy link
Member

Although this is a kind of improvement, this will reduce our maintenance cost (especially on LTS branch). So, I backported to branch-2.4.

@HeartSaVioR
Copy link
Contributor Author

Thanks all for reviewing and merging!

@HeartSaVioR HeartSaVioR deleted the MINOR-always-log-generated-code-on-fail-to-compile-in-unit-testing branch September 19, 2019 19:58
scunniff pushed a commit to scunniff/nomad-spark that referenced this pull request Nov 10, 2020
… in case of compile error on generated code in UT

### What changes were proposed in this pull request?

This patch proposes to change the log level of logging generated code in case of compile error being occurred in UT. This would help to investigate compilation issue of generated code easier, as currently we got exception message of line number but there's no generated code being logged actually (as in most cases of UT the threshold of log level is at least WARN).

### Why are the changes needed?

This would help investigating issue on compilation error for generated code in UT.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

N/A

Closes apache#25835 from HeartSaVioR/MINOR-always-log-generated-code-on-fail-to-compile-in-unit-testing.

Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit eee2e02)
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants