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

Flink: Maintenance - MonitorSource #10308

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Flink: Maintenance - MonitorSource #10308

merged 4 commits into from
Jun 6, 2024

Conversation

pvary
Copy link
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
Contributor

Please link the design doc in the PR description

Copy link
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
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
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/junit5#1786

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

Copy link
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
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
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
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
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
13 checks passed
@pvary
Copy link
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
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.

None yet

4 participants