Build: Enable revapi on core/parquet/orc/common/data modules & fix API breaks #6146
Conversation
nastra
changed the title
nastra
force-pushed
the
revapi-on-other-modules
branch
from
ad0f5b7 to
2b7aeaa
Compare
| * Snapshot)}. | ||
| */ | ||
| @Deprecated | ||
| public void validate(TableMetadata currentMetadata) { |
There was a problem hiding this comment.
we need to add separate Deprecations for validate() and apply here and in a few other places, because their visibility has been public and not protected (as it is defined in SnapshotProducer)
|
I've also created #6155 to remove all of those deprecated methods once 1.1.0 has been released |
nastra
force-pushed
the
revapi-on-other-modules
branch
3 times, most recently
from
a199e5a to
bbf1731
Compare
nastra
changed the title
.palantir/revapi.yml
Outdated
| justification: "new API method" | ||
| - code: "java.method.addedToInterface" | ||
| new: "method org.apache.iceberg.TableScan org.apache.iceberg.TableScan::useRef(java.lang.String)" | ||
| justification: "Adding table scan APIs to support scanning from refs" |
There was a problem hiding this comment.
Can we add a default implementation that throws an exception? That way we avoid a linker error and can signal that the implementation just doesn't support this configuration method.
There was a problem hiding this comment.
done, but this is something that was already shipped with 0.14.0 btw
.palantir/revapi.yml
Outdated
| justification: "Adding table scan APIs to support scanning from refs" | ||
| - code: "java.method.addedToInterface" | ||
| new: "method org.apache.iceberg.actions.MigrateTable org.apache.iceberg.actions.MigrateTable::dropBackup()" | ||
| justification: "Adding new functionality to allow for dropping backup table" |
There was a problem hiding this comment.
Same here, we should add a default implementation that throws an exception to avoid needing to suppress this.
There was a problem hiding this comment.
done, but this is something that was already shipped with 0.14.0 btw
| */ | ||
| @Deprecated | ||
| @Override | ||
| public <T extends RESTResponse> T delete( |
There was a problem hiding this comment.
Do we need to deprecate this? It seems like a reasonable thing to just keep it.
There was a problem hiding this comment.
I have removed the deprecation for now, but I would still consider deprecating it. We have 4 different delete methods in RESTClient. I think just keeping the one with queryParams would be ok
nastra
force-pushed
the
revapi-on-other-modules
branch
2 times, most recently
from
eb51a61 to
087ff88
Compare
nastra
force-pushed
the
revapi-on-other-modules
branch
from
087ff88 to
ef8dca7
Compare
nastra
force-pushed
the
revapi-on-other-modules
branch
from
ef8dca7 to
09b9435
Compare
|
Thanks, @nastra! Really great to have this in. |
5 participants
fixes #6144