Skip to content

Core: Add EnvironmentContext to commit summary#9273

Merged
amogh-jahagirdar merged 1 commit into
apache:mainfrom
manuzhang:rewrite_summary_spark_app_id
Apr 9, 2024
Merged

Core: Add EnvironmentContext to commit summary#9273
amogh-jahagirdar merged 1 commit into
apache:mainfrom
manuzhang:rewrite_summary_spark_app_id

Conversation

@manuzhang

@manuzhang manuzhang commented Dec 11, 2023

Copy link
Copy Markdown
Member

Currently, it's not easy to find the Spark application that has run rewrite files actions. This patch adds application id to commit summary via EnviromentContext

@manuzhang

Copy link
Copy Markdown
Member Author

@amogh-jahagirdar @nastra could you please take a look?

@manuzhang

Copy link
Copy Markdown
Member Author

The failed check is due to a flaky test. I created #9294 to fix it.

@manuzhang manuzhang force-pushed the rewrite_summary_spark_app_id branch 3 times, most recently from 9144372 to 62ec68a Compare February 19, 2024 12:54
@manuzhang manuzhang requested a review from nastra February 20, 2024 01:41
Comment thread core/src/main/java/org/apache/iceberg/SnapshotProducer.java
@manuzhang manuzhang force-pushed the rewrite_summary_spark_app_id branch from 62ec68a to b0363ef Compare February 20, 2024 16:22
@manuzhang manuzhang changed the title Spark 3.5: Add Spark application id to summary of RewriteDataFilesSparkAction Core: Add EnvironmentContext to commit summary Feb 21, 2024

@nastra nastra left a comment

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.

LGTM once the test is slightly updated. I think it makes sense to have engine-specific info in the snapshot summary. That way we know from which engine a snapshot has been created from. I'm curious what others think here

/cc @amogh-jahagirdar @aokolnychyi @RussellSpitzer @jackye1995

@manuzhang manuzhang force-pushed the rewrite_summary_spark_app_id branch from b0363ef to 8284c12 Compare February 22, 2024 08:04
@manuzhang manuzhang force-pushed the rewrite_summary_spark_app_id branch from 8284c12 to a57a15f Compare February 22, 2024 10:16
@manuzhang manuzhang force-pushed the rewrite_summary_spark_app_id branch from a57a15f to 98f9f60 Compare March 4, 2024 11:15
@manuzhang

Copy link
Copy Markdown
Member Author

@nastra how can we move this forward?

@manuzhang manuzhang force-pushed the rewrite_summary_spark_app_id branch from 98f9f60 to a8ce2b0 Compare March 26, 2024 05:18
@nastra

nastra commented Mar 26, 2024

Copy link
Copy Markdown
Contributor

@amogh-jahagirdar or @RussellSpitzer can you guys also take a look at this please?

@manuzhang manuzhang force-pushed the rewrite_summary_spark_app_id branch from a8ce2b0 to efea382 Compare April 9, 2024 10:21
@amogh-jahagirdar

Copy link
Copy Markdown
Contributor

Sorry for the delay on reviewing this @manuzhang thanks for adding this, I think the environment details in the summary will be quite helpful. I'll go ahead and merge. Thanks @nastra @ajantha-bhat for reviewing!

@amogh-jahagirdar amogh-jahagirdar merged commit 81bb0d4 into apache:main Apr 9, 2024
@manuzhang manuzhang deleted the rewrite_summary_spark_app_id branch April 9, 2024 15:23
sasankpagolu pushed a commit to sasankpagolu/iceberg that referenced this pull request Oct 27, 2024
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
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.

4 participants