OpenAPI: Deprecate snapshot-id of SetStatisticsUpdate #12010
Conversation
c-thiel
changed the title
.palantir/revapi.yml
Outdated
| @@ -1146,6 +1146,10 @@ acceptedBreaks: | |||
| \ org.apache.iceberg.TableMetadata)" | |||
| justification: "Removing deprecated code" | |||
| "1.7.0": | |||
| org.apache.iceberg:iceberg-api: | |||
There was a problem hiding this comment.
we can't break the API. I commented in https://github.com/apache/iceberg/pull/12010/files#r1922279411 on how to avoid this, so the changes here can be removed
There was a problem hiding this comment.
Thanks @nastra - fixed in the latest commit
| @@ -32,10 +32,22 @@ public SetStatistics(TableOperations ops) { | |||
| this.ops = ops; | |||
| } | |||
|
|
|||
| /** | |||
| * Add a new schema. | |||
There was a problem hiding this comment.
fixed in latest commit
Co-authored-by: Eduard Tudenhoefner <[email protected]>
| @@ -27,9 +27,21 @@ public interface UpdateStatistics extends PendingUpdate<List<StatisticsFile>> { | |||
| * the snapshot if any exists. | |||
| * | |||
| * @return this for method chaining | |||
| * @deprecated since 1.8.0, will be removed 1.9.0 or 2.0.0, use setStatistics(statisticsFile). | |||
There was a problem hiding this comment.
sorry I missed this during the first review, but this should be using proper javadoc: use {@link #setStatistics(StatisticsFile)}
There was a problem hiding this comment.
fixed in latest commit
| /** | ||
| * Set the statistics file for a snapshot. | ||
| * | ||
| * @deprecated since 1.8.0, will be removed in 1.9.0 or 2.0.0, use setStatistics(statisticsFile). |
There was a problem hiding this comment.
same as above: use {@link #setStatistics(StatisticsFile)}
There was a problem hiding this comment.
fixed in latest commit
| @@ -2890,6 +2889,9 @@ components: | |||
| snapshot-id: | |||
| type: integer | |||
| format: int64 | |||
| description: | |||
There was a problem hiding this comment.
this should probably also have a deprecated: true right before this line
There was a problem hiding this comment.
fixed in latest commit
|
@nastra ready for another round. |
Yes I think we should have a quick formal vote so that people are aware of this change |
| * the snapshot if any exists. | ||
| * | ||
| * @return this for method chaining | ||
| */ |
There was a problem hiding this comment.
nit: should we mention in the docstring that the snapshot id is coming from the statistics file?
There was a problem hiding this comment.
I like this - added a sentence to the comment.
Following the discussion on the Mailing List:
https://lists.apache.org/thread/phxjz196zbzg0fjpfkmnj0fpkshgh9z0
I would appreciate a more thorough look at the Java code changes, it's my first deprecation in iceberg and java is not my primary language.