Skip to content

[SPARK-37578][SQL] Update task metrics from ds v2 custom metrics#35028

Closed
viirya wants to merge 5 commits intoapache:masterfrom
viirya:SPARK-37578
Closed

[SPARK-37578][SQL] Update task metrics from ds v2 custom metrics#35028
viirya wants to merge 5 commits intoapache:masterfrom
viirya:SPARK-37578

Conversation

@viirya
Copy link
Copy Markdown
Member

@viirya viirya commented Dec 26, 2021

What changes were proposed in this pull request?

This patch proposes to update task metrics from datasource v2 custom metrics.

Why are the changes needed?

We have updated task metrics such as bytesWritten and recordsWritten from built-in data source v2 format e.g. Parquet. For other data source v2 formats, we can define some builtin metric names so Spark can also recognize them and update task metrics.

Does this PR introduce any user-facing change?

Yes. Some special DS v2 custom metrics are updated to task metrics.

How was this patch tested?

Added unit test.

@viirya viirya force-pushed the SPARK-37578 branch 2 times, most recently from cef027f to 832453a Compare December 27, 2021 09:01
@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 27, 2021

cc @cloud-fan

Option(TaskContext.get()).map(_.taskMetrics().outputMetrics).foreach { outputMetrics =>
metricName match {
case "bytesWritten" => outputMetrics.setBytesWritten(metricValue)
case "recordsWritten" => outputMetrics.setRecordsWritten(metricValue)
Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun Dec 27, 2021

Choose a reason for hiding this comment

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

Could you add the default no-op case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yea, I'll add it. Thanks.


private[spark] val NUM_ROWS_PER_UPDATE = 100

private[spark] val builtInOutputMetrics = Set("bytesWritten", "recordsWritten")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

builtInOutputMetrics -> BUILTIN_OUTPUT_METRICS?

Copy link
Copy Markdown
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. Thank you!


// Wait until the new execution is started and being tracked.
eventually(timeout(10.seconds), interval(10.milliseconds)) {
assert(statusStore.executionsCount() >= oldCount)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert(statusStore.executionsCount() >= oldCount)
assert(statusStore.executionsCount() > oldCount)

test("SPARK-37578: Update output metrics from Datasource v2") {
withTempDir { dir =>
val statusStore = spark.sharedState.statusStore
val oldCount = statusStore.executionsList().size
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this different from statusStore.executionsCount()?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is the same, I think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

then statusStore.executionsCount() looks more efficient? It's also used in the code below.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yea, I will change to executionsCount.

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 29, 2021

@cloud-fan Thanks. Any more comments?

Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan left a comment

Choose a reason for hiding this comment

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

LGTM, pending CI

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 29, 2021

All tests in GA are passed. It only failed on Python lint check.

@dongjoon-hyun
Copy link
Copy Markdown
Member

You can ignore Python linter error~

@viirya
Copy link
Copy Markdown
Member Author

viirya commented Dec 29, 2021

Yea, thanks @dongjoon-hyun @cloud-fan . Merging to master.

@viirya viirya closed this in c808077 Dec 29, 2021
@viirya viirya deleted the SPARK-37578 branch December 27, 2023 18:26
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.

3 participants