-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
OpenAPI: Add RemovePartitionSpecsUpdate REST update type #10846
OpenAPI: Add RemovePartitionSpecsUpdate REST update type #10846
Conversation
3154e8c
to
35d5a52
Compare
bf326ba
to
46ef535
Compare
46ef535
to
1bdefbd
Compare
@@ -2265,6 +2265,7 @@ components: | |||
remove-statistics: '#/components/schemas/RemoveStatisticsUpdate' | |||
set-partition-statistics: '#/components/schemas/SetPartitionStatisticsUpdate' | |||
remove-partition-statistics: '#/components/schemas/RemovePartitionStatisticsUpdate' | |||
remove-partition-specs: '#/components/schemas/RemovePartitionSpecsUpdate' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about remove-unused-partitioned-specs
/remove-unused-specs
, emphasizing the unused
part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, I'm neutral about the name, this name is more general and expresses the intention well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think I would prefer to have the name be generic. While the update type would primarily be used for the RemoveUnusedSpecs option, I don't think that particular use case needs to be explicitly outlined in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also go for a more generic name, would skip unused
for now.
1bdefbd
to
9d9e32b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's start a VOTE/discussion thread on the dev list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@advancedxy I put up #10848 to clarify that REST servers must throw a 400 error code if they get an update they cannot understand. That was prompted by this change since I think we want to to make it clear how existing servers should behave if they get this new RemovePartitionSpecsUpdate
and are unable to process it.
I think we should probably wait for that to go in before proceeding here, just so the community is on the same page.
Based on the mailing list thread https://lists.apache.org/thread/9g9nywj7xpxkf3l4wz9p5qgszm9x1r13 doesn't seem like there are major objections.
Of course. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Looks like we have enough binding votes both here and in devlist: https://lists.apache.org/thread/5pt5g4q9zt856cpscob37y30mczlf9rx, and the thread is open for quite a while now. I will go ahead to merge it. Thanks Amogh! |
* main: (208 commits) Docs: Fix Flink 1.20 support versions (apache#11065) Flink: Fix compile warning (apache#11072) Docs: Initial committer guidelines and requirements for merging (apache#10780) Core: Refactor ZOrderByteUtils (apache#10624) API: implement types timestamp_ns and timestamptz_ns (apache#9008) Build: Bump com.google.errorprone:error_prone_annotations (apache#11055) Build: Bump mkdocs-material from 9.5.33 to 9.5.34 (apache#11062) Flink: Backport PR apache#10526 to v1.18 and v1.20 (apache#11018) Kafka Connect: Disable publish tasks in runtime project (apache#11032) Flink: add unit tests for range distribution on bucket partition column (apache#11033) Spark 3.5: Use FileGenerationUtil in PlanningBenchmark (apache#11027) Core: Add benchmark for appending files (apache#11029) Build: Ignore benchmark output folders across all modules (apache#11030) Spec: Add RemovePartitionSpecsUpdate REST update type (apache#10846) Docs: bump latest version to 1.6.1 (apache#11036) OpenAPI, Build: Apply spotless to testFixtures source code (apache#11024) Core: Generate realistic bounds in benchmarks (apache#11022) Add REST Compatibility Kit (apache#10908) Flink: backport PR apache#10832 of inferring parallelism in FLIP-27 source (apache#11009) Docs: Add Druid docs url to sidebar (apache#10997) ...
See #10755
A new update type is required to be able to enable removing inactive partition specs.