-
Notifications
You must be signed in to change notification settings - Fork 3k
Core: Change removal of deprecations to 1.12.0 #14392
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,7 +81,7 @@ private TransientEncryptionState(KeyManagementClient kmsClient, List<EncryptedKe | |
| private transient volatile SecureRandom lazyRNG = null; | ||
|
|
||
| /** | ||
| * @deprecated will be removed in 2.0. | ||
| * @deprecated will be removed in 1.12.0. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CC: @ggershinsky - is there any particular reason to keep these to 2.0?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be ok to remove earlier. |
||
| */ | ||
| @Deprecated | ||
| public StandardEncryptionManager( | ||
|
|
@@ -141,7 +141,7 @@ private SecureRandom workerRNG() { | |
| } | ||
|
|
||
| /** | ||
| * @deprecated will be removed in 2.0. | ||
| * @deprecated will be removed in 1.12.0. | ||
| */ | ||
| @Deprecated | ||
| public ByteBuffer wrapKey(ByteBuffer secretKey) { | ||
|
|
@@ -154,7 +154,7 @@ public ByteBuffer wrapKey(ByteBuffer secretKey) { | |
| } | ||
|
|
||
| /** | ||
| * @deprecated will be removed in 2.0. | ||
| * @deprecated will be removed in 1.12.0. | ||
| */ | ||
| @Deprecated | ||
| public ByteBuffer unwrapKey(ByteBuffer wrappedSecretKey) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -140,7 +140,7 @@ public void invalidate(String key) { | |
| } | ||
|
|
||
| /** | ||
| * @deprecated since 1.7.0, will be removed in 2.0.0; This method does only best-effort | ||
| * @deprecated since 1.7.0, will be removed in 1.12.0; This method does only best-effort | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is an alternative way to empty the cache?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @findepi Do you know any alternative way to invalidate all entries in the cache? @pvary we discussed that we may want to leave the targeted removal for 2.0.0. However, this is a functionality we'd like the users to come off (due to the race condition) I think we should remove asap, so 1.12.0 still makes sense to me. WDYT?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Depends on the goal. In trino we have additional layer on top of Guava caches that solves this problem. Here we use caffeine, but last time i checked, it had the same invalidation race problem and the author's feedback was that it was fundamentally not solvable. Here i'd hope that invalidation isn't really needed for anything and we can remove the API. |
||
| * invalidation and is susceptible to a race condition. If the caller changed the state that | ||
| * could be cached (perhaps files on the storage) and calls this method, there is no guarantee | ||
| * that the cache will not contain stale entries some time after this method returns. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,7 +107,7 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog | |
| private static final String DEFAULT_FILE_IO_IMPL = "org.apache.iceberg.io.ResolvingFileIO"; | ||
|
|
||
| /** | ||
| * @deprecated will be removed in 2.0.0. Use {@link | ||
| * @deprecated will be removed in 1.12.0. Use {@link | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one should be fine to remove before 2.0.0, since the property was only moved to a different class |
||
| * org.apache.iceberg.rest.RESTCatalogProperties#PAGE_SIZE} instead. | ||
| */ | ||
| @Deprecated public static final String REST_PAGE_SIZE = "rest-page-size"; | ||
|
|
||
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.
did the deprecation removal rules change?
it looks like we considered not ok to remove a public API in a minor update, and now we conclude it's ok.
has this been discussed somewhere already?
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.
There was a discussion about this in one of the community syncs. The conclusion was that we give guarantees for a whole major version in the api/ module, however in the core/ module we could be less strict and provide guarantees for one minor release. Technically deprecate in the upcoming minor, drop in the one after that is OK in core. Hence, this PR collects all the deprecations in core/ marking removal for 2.0.0 to revisit them and drop earlier if possible.
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.
is this written down somewhere so that we can reference the rules of the games when deprecating?
community syncs meeting notes are not normative, we should capture important conclusions somewhere
btw, i don't think this 1 minor release guarantee means much in practice. maven world reality is that minor version update may just happen when combining various artifacts for the final delivery, so removing an API may obviously lead to MethodDefNotFoundError. This may be a fair trade off, i just hope it's a conscious one
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.
Thanks for taking a look, @findepi !
Here are the docs for the api guarantees in different modules. It mentions one minor version guarantee in core module.