You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ajantha-bhat (via GitHub)" <gi...@apache.org> on 2025/04/28 06:56:08 UTC
[PR] Build: Let revapi compare against 1.9.0 [iceberg]
ajantha-bhat opened a new pull request, #12912:
URL: https://github.com/apache/iceberg/pull/12912
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2835404761
> @nastra: Do we need to revert #12882 and revaluate whether the breakage is ok? or I can just update the revapi.yml with these breakages?
We just added those APIs so I think we can probably add the breakage to RevAPI. @danielcweeks @amogh-jahagirdar wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2835154929
Hm this is quite unfortunate that https://github.com/apache/iceberg/pull/12882 went in right in between voting on 1.9.0 happened, which means that the API breakage wasn't detected as a breakage when it was merged
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#discussion_r2066892520
##########
core/src/main/java/org/apache/iceberg/io/StorageCredential.java:
##########
@@ -29,18 +29,27 @@ public interface StorageCredential extends Serializable {
String prefix();
- SerializableMap<String, String> config();
+ /**
+ * @deprecated since 1.10.0, will be removed in 1.11.0; use {@link #serializableConfig()} instead.
+ */
+ @Deprecated
+ default Map<String, String> config() {
+ return serializableConfig();
+ }
+
+ SerializableMap<String, String> serializableConfig();
Review Comment:
It is not breaking as we deprecate the old interface and adding new interface here. But yeah, revAPI considers new interface addition also as a breaking change.
I like your approach in #12930
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2839450032
closing in the favor of #12930
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#discussion_r2066481290
##########
core/src/main/java/org/apache/iceberg/io/StorageCredential.java:
##########
@@ -29,18 +29,27 @@ public interface StorageCredential extends Serializable {
String prefix();
- SerializableMap<String, String> config();
+ /**
+ * @deprecated since 1.10.0, will be removed in 1.11.0; use {@link #serializableConfig()} instead.
+ */
+ @Deprecated
+ default Map<String, String> config() {
+ return serializableConfig();
+ }
+
+ SerializableMap<String, String> serializableConfig();
Review Comment:
I don't think introducing this helps either, because it also introduces breaking API changes
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2839403366
FYI I've opened https://github.com/apache/iceberg/pull/12930 as an alternative to this so that we can try and avoid the API breakage will still making ser/de properly work with Kryo
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2837694580
> Let me add that I feel like we're working around Immutables here. The interface itself should be fine with Map<>. Is there a way to tell Immmutables to use a specific concrete implementation or is that not supported?
>
> It may be moot since the release has already passed, but we could always fix this and make the interface represent the original type.
The underlying issue is that Immutables will always create an unmodifiable Map when the API definition is `Map<String, String>`, which Kryo can't deal with, since Kryo adds back elements to the collection after deserialization. There is some hope that Kryo will eventually be able to deal with unmodifiable collections without having to use a separate kryo-serializers lib (https://github.com/EsotericSoftware/kryo/pull/1154) but we're not there yet and maybe the better option would have been to not make `StorageCredential` use Immutables in the first place due to Kryo.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2838689366
@nastra and @danielcweeks, thanks for the context and inputs. I have deprecated the existing interface and added new. That way it is clean and as per our guidelines. Please take another look.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2836350461
Let me add that I feel like we're working around Immutables here. The interface itself should be fine with Map<>. Is there a way to tell Immmutables to use a specific concrete implementation or is that not supported?
It may be moot since the release has already passed, but we could always fix this and make the interface represent the original type.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat closed pull request #12912: Build: Let revapi compare against 1.9.0
URL: https://github.com/apache/iceberg/pull/12912
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2835213592
@nastra: Do we need to revert #12882 and revaluate whether the breakage is ok? or I can just update the revapi.yml with these breakages?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Build: Let revapi compare against 1.9.0 [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #12912:
URL: https://github.com/apache/iceberg/pull/12912#issuecomment-2836274559
I think we should be ok accepting the revapi violation here. The storage credential serialization is scoped to runtime and the serialization through REST API is json, so I don't think this is going to be an incompatible change in practice.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org