wypoon commented on code in PR #8617:
URL: https://github.com/apache/iceberg/pull/8617#discussion_r1334567304
##########
.palantir/revapi.yml:
##########
@@ -833,6 +829,13 @@ acceptedBreaks:
- code: "java.class.removed"
old: "interface org.apache.iceberg.actions.RewritePositionDeleteStrategy"
justification: "Removing deprecated code"
+ - code: "java.method.abstractMethodAdded"
Review Comment:
Yes, your suggestion was followed in
https://github.com/apache/iceberg/pull/6799 and I haven't reverted it there.
I think @ConeyLiu's point is that the new `newAppender` method doesn't break
the API because it is not abstract -- it calls the old `newAppender`. However,
the old `newAppender` is deprecated and when it is removed, the new
`newAppender` then has to be changed. There is no natural implementation for
it, so it should be abstract. Of course, one could do something else, such as
return null or throw an `UnsupportedOperationException`.
If anyone out there actually writes their own `ManifestWriter` subclass,
they would need to implement a `newAppender(PartitionSpec, OutputFile, String,
Integer)` method. We avoid breaking the API now, which allows them to get way
without implementing the new method, but when `newAppender(PartitionSpec,
OutputFile)` is then removed, that will break them (even if we do not
technically break the API by keeping `newAppender(PartitionSpec, OutputFile,
String, Integer)` non-abstract and throwing an `UnsupportedOperationException`
-- it effectively breaks them).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
