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.

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

Flink: Maintenance - TableManager + ExpireSnapshots #11144

Merged
merged 12 commits into from
Nov 4, 2024
Merged

Flink: Maintenance - TableManager + ExpireSnapshots #11144

merged 12 commits into from
Nov 4, 2024

Conversation

pvary
Copy link
Contributor

@pvary pvary commented Sep 16, 2024

TableManager builder implementation along with the first maintenance task to provide context.
https://docs.google.com/document/d/16g3vR18mVBy8jbFaLjf2JwAANuYOmIwr15yDDxovdnA/edit#heading=h.yd2vbtnf7z6w

@github-actions github-actions bot added the flink label Sep 16, 2024
@pvary
Copy link
Contributor Author

pvary commented Sep 16, 2024

@stevenzwu: This PR become quite sizeable. I still think that it is better to keep it as one to provide context for some of the decisions made during the definition of the MaintenanceTaskBuilder.

If you have time we could discuss offline the review strategy, and whether to split this PR to smaller ones.

Thanks,
Peter

@pvary pvary force-pushed the flow branch 3 times, most recently from a1dabe5 to 96322c5 Compare September 16, 2024 09:38
stevenzwu
stevenzwu reviewed Sep 16, 2024
View reviewed changes
.../flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/MaintenanceTaskBuilder.java Outdated Show resolved Hide resolved
...k/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/ExpireSnapshots.java Outdated Show resolved Hide resolved
...k/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/ExpireSnapshots.java Outdated Show resolved Hide resolved
...k/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/ExpireSnapshots.java Outdated Show resolved Hide resolved
...k/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/ExpireSnapshots.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/TableMaintenance.java Outdated Show resolved Hide resolved
...nk/src/main/java/org/apache/iceberg/flink/maintenance/operator/ExpireSnapshotsProcessor.java Outdated Show resolved Hide resolved
...1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/AsyncDeleteFiles.java Outdated Show resolved Hide resolved
...k/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/ExpireSnapshots.java Outdated Show resolved Hide resolved
...nk/src/main/java/org/apache/iceberg/flink/maintenance/operator/ExpireSnapshotsProcessor.java Outdated Show resolved Hide resolved
stevenzwu
stevenzwu reviewed Sep 17, 2024
View reviewed changes
.../flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/MaintenanceTaskBuilder.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/TableMaintenance.java Outdated
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.iceberg.flink.maintenance.stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if users would interpret stream sub package as pubic APIs. It is better to use proper Java class scope for that purpose. public classes are public and non-public classes can be package private.

stevenzwu
stevenzwu reviewed Sep 17, 2024
View reviewed changes
...1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/AsyncDeleteFiles.java Outdated Show resolved Hide resolved
...nk/src/main/java/org/apache/iceberg/flink/maintenance/operator/ExpireSnapshotsProcessor.java Show resolved Hide resolved
...nk/src/main/java/org/apache/iceberg/flink/maintenance/operator/ExpireSnapshotsProcessor.java Show resolved Hide resolved
...nk/src/main/java/org/apache/iceberg/flink/maintenance/operator/ExpireSnapshotsProcessor.java Outdated Show resolved Hide resolved
...nk/src/main/java/org/apache/iceberg/flink/maintenance/operator/ExpireSnapshotsProcessor.java Show resolved Hide resolved
.../v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/TableMaintenance.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/TableMaintenance.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/TableMaintenance.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/TableMaintenance.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/stream/TableMaintenance.java Outdated Show resolved Hide resolved
stevenzwu
stevenzwu reviewed Sep 18, 2024
View reviewed changes
.../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestAsyncDeleteFiles.java Outdated Show resolved Hide resolved
.../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestAsyncDeleteFiles.java Outdated Show resolved Hide resolved
.../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestAsyncDeleteFiles.java Outdated Show resolved Hide resolved
.../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestAsyncDeleteFiles.java Outdated Show resolved Hide resolved
.../flink/src/test/java/org/apache/iceberg/flink/maintenance/operator/TestAsyncDeleteFiles.java Outdated Show resolved Hide resolved
...20/flink/src/test/java/org/apache/iceberg/flink/maintenance/stream/TestTableMaintenance.java Outdated Show resolved Hide resolved
...20/flink/src/test/java/org/apache/iceberg/flink/maintenance/stream/TestTableMaintenance.java Outdated Show resolved Hide resolved
...20/flink/src/test/java/org/apache/iceberg/flink/maintenance/stream/TestTableMaintenance.java Outdated Show resolved Hide resolved
...20/flink/src/test/java/org/apache/iceberg/flink/maintenance/stream/TestTableMaintenance.java Outdated Show resolved Hide resolved
...20/flink/src/test/java/org/apache/iceberg/flink/maintenance/stream/TestTableMaintenance.java Outdated Show resolved Hide resolved
@pvary pvary force-pushed the flow branch 5 times, most recently from fa56618 to 2403f44 Compare September 18, 2024 20:44
stevenzwu
stevenzwu reviewed Sep 26, 2024
View reviewed changes
flink/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/ExpireSnapshots.java Outdated Show resolved Hide resolved
.../flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/DeleteFilesProcessor.java Outdated Show resolved Hide resolved
.../flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/DeleteFilesProcessor.java Outdated Show resolved Hide resolved
.../flink/src/main/java/org/apache/iceberg/flink/maintenance/operator/DeleteFilesProcessor.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/test/java/org/apache/iceberg/flink/maintenance/api/TestExpireSnapshots.java Outdated Show resolved Hide resolved
.../v1.20/flink/src/test/java/org/apache/iceberg/flink/maintenance/api/TestExpireSnapshots.java Show resolved Hide resolved
...k/v1.20/flink/src/test/java/org/apache/iceberg/flink/maintenance/api/TestMaintenanceE2E.java Show resolved Hide resolved
@pvary
Copy link
Contributor Author

pvary commented Sep 26, 2024

@netvl: I'm struggling to find the relevant comment (there are too many of them already), but I would like to ask you to check if the proposed MaintenanceTaskBuilder would serve your needs. You can see in the ExpireSnapshots class how it is ment to be used.

Thanks,
Peter

@pvary
Copy link
Contributor Author

pvary commented Sep 26, 2024

@rodmeneses: If you have time, I would like to ask you to review the PR.
Thanks,
Peter

This was referenced Oct 1, 2024
Closed
Closed
stevenzwu
stevenzwu reviewed Oct 23, 2024
View reviewed changes
....20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/MaintenanceTaskBuilder.java Outdated Show resolved Hide resolved
....20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/MaintenanceTaskBuilder.java Outdated Show resolved Hide resolved
flink/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/ExpireSnapshots.java Show resolved Hide resolved
flink/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/TableMaintenance.java Outdated Show resolved Hide resolved
flink/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/TableMaintenance.java Outdated Show resolved Hide resolved
flink/v1.20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/TableMaintenance.java Outdated Show resolved Hide resolved
stevenzwu
stevenzwu approved these changes Oct 24, 2024
View reviewed changes
....20/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/MaintenanceTaskBuilder.java Outdated
TableLoader newTableLoader,
String defaultUidSuffix,
String defaultSlotSharingGroup,
int mainParallelism) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also be defaultParallelism?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@pvary pvary merged commit 0669bcb into apache:main Nov 4, 2024
20 checks passed
@pvary
Copy link
Contributor Author

pvary commented Nov 4, 2024

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

pvary pushed a commit to pvary/iceberg that referenced this pull request Nov 5, 2024
Flink: Port apache#11144 to v1.19
61db4fd
@pvary pvary mentioned this pull request Nov 5, 2024
Merged
pvary added a commit that referenced this pull request Nov 8, 2024
@pvary
Flink: Port #11144 to v1.19 (#11473)
3da64d3
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
@pvary @zachdisc
Flink: Maintenance - TableManager + ExpireSnapshots (apache#11144)
ec81acf
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
@pvary @zachdisc
Flink: Port apache#11144 to v1.19 (apache#11473)
a8b5d57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodmeneses rodmeneses rodmeneses left review comments

@netvl netvl netvl left review comments

@stevenzwu stevenzwu stevenzwu approved these changes

Assignees
No one assigned
Labels
flink
Projects
Flink Table Maintenance
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pvary @netvl @stevenzwu @rodmeneses