Skip to content

Flink: Maintenance - MonitorSource#10308

Merged
pvary merged 4 commits into
apache:mainfrom
pvary:table_change
Jun 6, 2024
Merged

Flink: Maintenance - MonitorSource#10308
pvary merged 4 commits into
apache:mainfrom
pvary:table_change

Conversation

@pvary
Copy link
Copy Markdown
Contributor

@pvary pvary commented May 10, 2024

Implements a monitor source which emits TableChange events based on the new commits to the Iceberg table.
This will be used for Maintenance Task scheduling as described in the design doc. The Scheduling paragraph describes the require parameters of the TableChange object. Also the Monitor paragraph adds some more details.

Implements #10300

@rodmeneses
Copy link
Copy Markdown
Contributor

Please link the design doc in the PR description

Copy link
Copy Markdown
Contributor

@rodmeneses rodmeneses left a comment

Choose a reason for hiding this comment

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

some minor comments, otherwise LGTM


// Add temporary dir as a warehouse location
try {
this.warehouse = Files.createTempDirectory("warehouse");
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.

do we need to pass in a @TempDir that can auto clean up files/dirs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In Junit5 mixing extensions with @TempDir is not supported.
See: junit-team/junit-framework#1786

I found the proposed solution much more ugly than creating a temp dir using the java api

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.

My concern is that the temp dir (and files under it) wasn't cleaned up at the end of the test. at least, we should consider deleteOnExit()

https://stackoverflow.com/questions/15022219/does-files-createtempdirectory-remove-the-directory-after-jvm-exits-normally

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is taken care in the afterEach method:

Files.walk(warehouse).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);

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.

ah. I missed the afterEach part. it might still make sense to add deleteOnExit as fallback, in case afterEach method throws an exception before the file cleanup step.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

deleteOnExit will only work if the directory is empty. See: https://docs.oracle.com/javase/7/docs/api/java/io/File.html#delete%28%29

I don't think it would help much adding this to the code.

@pvary pvary force-pushed the table_change branch 4 times, most recently from c1c188d to 41d1b29 Compare June 4, 2024 11:58
Copy link
Copy Markdown
Contributor

@stevenzwu stevenzwu left a comment

Choose a reason for hiding this comment

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

LGTM. have one minor comment and will leave the decision to you on that one

@pvary pvary merged commit c7d3ef4 into apache:main Jun 6, 2024
@pvary
Copy link
Copy Markdown
Contributor Author

pvary commented Jun 6, 2024

Merged to main.
Thanks for the review @rodmeneses and @stevenzwu!

@pvary pvary deleted the table_change branch June 6, 2024 07:51
jasonf20 pushed a commit to jasonf20/iceberg that referenced this pull request Aug 4, 2024
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

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants