Skip to content

[SPARK-34355][CORE][SQL][FOLLOWUP] Log commit time in all File Writer #31520

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.

Sign up for GitHub

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

Jump to bottom
Closed
AngersZhuuuu wants to merge 6 commits into from
Closed

[SPARK-34355][CORE][SQL][FOLLOWUP] Log commit time in all File Writer #31520

AngersZhuuuu wants to merge 6 commits into from

Conversation

@AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu AngersZhuuuu commented Feb 8, 2021 •
edited
Loading

What changes were proposed in this pull request?

When doing https://issues.apache.org/jira/browse/SPARK-34399 based on #31471
Found FileBatchWrite will use FileFormatWrite.processStates() too. We need log commit duration in other writer too.
In this pr:

  1. Extract a commit job method in SparkHadoopWriter
  2. address other commit writer

Why are the changes needed?

Does this PR introduce any user-facing change?

No

How was this patch tested?

No

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Feb 8, 2021

gentle ping @HeartSaVioR

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Feb 8, 2021

  1. Let's address SparkHadoopWriter as well.
  2. If these three occurrences are calling exactly the same method, probably extract it to the one of method of SparkHadoopWriter and let others call it.

@github-actions github-actions bot added the SQL label Feb 8, 2021
@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Feb 8, 2021

  1. Let's address SparkHadoopWriter as well.
  2. If these three occurrences are calling exactly the same method, probably extract it to the one of method of SparkHadoopWriter and let others call it.

Updated, how about current?

@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39596/

yikf
yikf reviewed Feb 8, 2021
View reviewed changes
core/src/main/scala/org/apache/spark/internal/io/SparkHadoopWriterUtils.scala Outdated
job: JobContext,
uuid: String,
committer: FileCommitProtocol,
commitMsgs: Seq[TaskCommitMessage]): Long = {
Copy link
Contributor

@yikf yikf Feb 8, 2021

Choose a reason for hiding this comment

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

The retrun value of commitJob seems not be used.

Copy link
Contributor Author

@AngersZhuuuu AngersZhuuuu Feb 8, 2021

Choose a reason for hiding this comment

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

The retrun value of commitJob seems not be used.

Will be used in #31522, revert this in current pr

@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39603/

@github-actions github-actions bot added the CORE label Feb 8, 2021
@AngersZhuuuu AngersZhuuuu changed the title [SPARK-34355][SQL][FOLLOWUP] Log FileBatchWrite's commit time too [SPARK-34355][CORE][SQL][FOLLOWUP] Log FileBatchWrite's commit time too Feb 8, 2021
@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39596/

@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39603/

@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39604/

@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39604/

@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Test build #135018 has finished for PR 31520 at commit 91d92f4.

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

@AngersZhuuuu AngersZhuuuu changed the title [SPARK-34355][CORE][SQL][FOLLOWUP] Log FileBatchWrite's commit time too [SPARK-34355][CORE][SQL][FOLLOWUP] Log commit time in all File Writer Feb 8, 2021
@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Test build #135013 has finished for PR 31520 at commit 171cefd.

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

@SparkQA
Copy link

SparkQA commented Feb 8, 2021

Test build #135021 has finished for PR 31520 at commit 555f5b4.

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

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Feb 8, 2021

ping @HeartSaVioR

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Feb 9, 2021

I'm sorry I haven't considered about the impact of extracting the code - logger. I'm not sure we are leveraging the information on classname difference for tracking log on committing in practice. Let's just make sure others are OK with this, otherwise I might ask you to unroll the change. Sorry again.

cc. @LuciferYang @HyukjinKwon @yaooqinn @dongjoon-hyun (reviewers on #31471)
I assume you have interest on the commit log message, and probably dealing with it in production. Would retaining the caller site help you or no matter which logger will log it?

@AngersZhuuuu
Copy link
Contributor Author

AngersZhuuuu commented Feb 9, 2021

I'm sorry I haven't considered about the impact of extracting the code - logger. I'm not sure we are leveraging the information on classname difference for tracking log on committing in practice. Let's just make sure others are OK with this, otherwise I might ask you to unroll the change. Sorry again.

Yea, the concern is reasonable, and I have revert them.

@SparkQA
Copy link

SparkQA commented Feb 9, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39639/

@SparkQA
Copy link

SparkQA commented Feb 9, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39639/

@SparkQA
Copy link

SparkQA commented Feb 9, 2021

Test build #135056 has finished for PR 31520 at commit 6c3bdc6.

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

@yaooqinn
Copy link
Member

yaooqinn commented Feb 9, 2021

logging separately make sense to me and thanks for pinging me @HeartSaVioR

@SparkQA
Copy link

SparkQA commented Feb 9, 2021

Test build #135057 has finished for PR 31520 at commit 7b7d2bd.

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

HeartSaVioR
HeartSaVioR approved these changes Feb 9, 2021
View reviewed changes
Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1

Given the PR removed the possible behavioral change, I'll go with merging.

@HeartSaVioR
Copy link
Contributor

HeartSaVioR commented Feb 9, 2021

Thanks @AngersZhuuuu for the contribution! Merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HeartSaVioR HeartSaVioR HeartSaVioR approved these changes

+1 more reviewer

@yikf yikf yikf left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CORE SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AngersZhuuuu @HeartSaVioR @SparkQA @yaooqinn @yikf