mystic-lama commented on code in PR #8293: URL: https://github.com/apache/iceberg/pull/8293#discussion_r1292575261
########## CONTRIBUTING.md: ########## @@ -108,6 +108,85 @@ Example: void sequenceNumber(long sequenceNumber); ``` +## Adding new functionality without breaking APIs +Ideally, we'd want to add new functionality without breaking existing APIs, especially within the scope of the API modules that are being checked by [Revapi](https://revapi.org/). + +Let's assume we'd want to add a `createBranch(String name)` method to the `ManageSnapshots` API. + +The most straight-forward way would be to add the below code: + +```java +public interface ManageSnapshots extends PendingUpdate<Snapshot> { + // existing code... + + // introduces an API-breaking change Review Comment: Minor nit: can we say // introduce an API-breaking change by adding the method or // introduce an API-breaking change by adding the below method ########## CONTRIBUTING.md: ########## @@ -108,6 +108,85 @@ Example: void sequenceNumber(long sequenceNumber); ``` +## Adding new functionality without breaking APIs +Ideally, we'd want to add new functionality without breaking existing APIs, especially within the scope of the API modules that are being checked by [Revapi](https://revapi.org/). + +Let's assume we'd want to add a `createBranch(String name)` method to the `ManageSnapshots` API. + +The most straight-forward way would be to add the below code: + +```java +public interface ManageSnapshots extends PendingUpdate<Snapshot> { + // existing code... + + // introduces an API-breaking change + ManageSnapshots createBranch(String name); +} +``` + +And then add the implementation: +```java +public class SnapshotManager implements ManageSnapshots { + // existing code... + + @Override + public ManageSnapshots createBranch(String name, long snapshotId) { + updateSnapshotReferencesOperation().createBranch(name, snapshotId); + return this; + } +} +``` + +### Checking for API breakages +Running `./gradlew revapi` will flag this as an API-breaking change: +``` +./gradlew revapi +> Task :iceberg-api:revapi FAILED +> Task :iceberg-api:showDeprecationRulesOnRevApiFailure FAILED + +1: Task failed with an exception. +----------- +* What went wrong: +Execution failed for task ':iceberg-api:revapi'. +> There were Java public API/ABI breaks reported by revapi: + + java.method.addedToInterface: Method was added to an interface. + + old: <none> + new: method org.apache.iceberg.ManageSnapshots org.apache.iceberg.ManageSnapshots::createBranch(java.lang.String) + + SOURCE: BREAKING, BINARY: NON_BREAKING, SEMANTIC: POTENTIALLY_BREAKING + + From old archive: <none> + From new archive: iceberg-api-1.4.0-SNAPSHOT.jar + + If this is an acceptable break that will not harm your users, you can ignore it in future runs like so for: + + * Just this break: Review Comment: newbie question: If we introduce a new API, the CI shall flag it. How does it get handled during automated build? -- 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]
