Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-47589][SQL]Hive-Thriftserver: Migrate logError with variables to structured logging framework #45936

Closed

Conversation

gengliangwang
Copy link
Member

What changes were proposed in this pull request?

Migrate logError with variables of Hive-thriftserver module to the structured logging framework.

Why are the changes needed?

To enhance Apache Spark's logging system by implementing structured logging.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UT

Was this patch authored or co-authored using generative AI tooling?

No

@gengliangwang
Copy link
Member Author

cc @dtenedor @panbingkun @itholic

@gengliangwang gengliangwang changed the title [SPARK-47589][CORE] Hive-thriftserver: Migrate logError with variables to structured logging framework [SPARK-47589][Thriftserver] Hive-thriftserver: Migrate logError with variables to structured logging framework Apr 8, 2024
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 (Pending CIs).

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-47589][Thriftserver] Hive-thriftserver: Migrate logError with variables to structured logging framework [SPARK-47589][SQL] Hive-thriftserver: Migrate logError with variables to structured logging framework Apr 8, 2024
@gengliangwang gengliangwang changed the title [SPARK-47589][SQL] Hive-thriftserver: Migrate logError with variables to structured logging framework [SPARK-47589][HIVE-THRIFTSERVER] Migrate logError with variables to structured logging framework Apr 8, 2024
@dongjoon-hyun
Copy link
Member

Oh, should this be [HIVE-THRIFTSERVER], @gengliangwang ?

@dongjoon-hyun
Copy link
Member

Just for my knowledge, may I ask why this should be different from the GitHub Action label, @gengliangwang ?

Screenshot 2024-04-08 at 15 18 28

@gengliangwang gengliangwang changed the title [SPARK-47589][HIVE-THRIFTSERVER] Migrate logError with variables to structured logging framework [SPARK-47589][SQL]Hive-Thriftserver: Migrate logError with variables to structured logging framework Apr 8, 2024
@gengliangwang
Copy link
Member Author

@dongjoon-hyun ok, I just changed back to [SQL]. Thanks for pointing it out!

@dongjoon-hyun
Copy link
Member

Oh, I didn't mean it was wrong here. I want to understand the organization of this log migration workitem. :)

@gengliangwang
Copy link
Member Author

gengliangwang commented Apr 8, 2024

the organization of this log migration workitem.

I am trying to split the tasks as per the "project" definition in SBT. For example, we got project hive/hive-thriftserver/catalyst/sql/etc from sbt.

Connectors are special. We can have 3 PRs to cover all of them, instead of migration for each connector.

There are around 30 migration tasks from https://issues.apache.org/jira/browse/SPARK-47240. The size seems reasonable. It is neither too large nor too small.

Copy link
Contributor

@itholic itholic left a comment

Choose a reason for hiding this comment

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

Left one question, but not a LGTM blocker.

@@ -142,7 +144,9 @@ private[hive] class SparkExecuteStatementOperation(
} catch {
case NonFatal(e) =>
setOperationException(new HiveSQLException(e))
logError(s"Error cancelling the query after timeout: $timeout seconds")
val timeout_ms = timeout * MILLIS_PER_SECOND
Copy link
Contributor

Choose a reason for hiding this comment

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

qq, just for my personal understanding: why do we convert second to millisecond here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gengliangwang
Copy link
Member Author

@dongjoon-hyun @itholic Thanks for the review. Merging to master

@panbingkun
Copy link
Contributor

+1, LGTM. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants