Build: Let revapi compare against 1.9.0

[ajantha-bhat]

No description provided.

[nastra]

Hm this is quite unfortunate that #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

[ajantha-bhat]

@nastra: Do we need to revert #12882 and re-evaluate whether the breakage is ok? or I can just update the revapi.yml with these breakages?

[nastra]

@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 the StorageCredential API so I think we can probably add the breakage to RevAPI, since we're really just changing the return type from Map<String, String to SerializableMap<String, String to fix an issue with Kryo. @danielcweeks @amogh-jahagirdar wdyt?

For reference, this is the API breakage that RevAPI complains about:

java.method.returnTypeChangedCovariantly: The return type changed covariantly from 'java.util.Map<java.lang.String, java.lang.String>' to 'org.apache.iceberg.util.SerializableMap<java.lang.String, java.lang.String>'.

  old: method java.util.Map<java.lang.String, java.lang.String> org.apache.iceberg.io.StorageCredential::config()
  new: method org.apache.iceberg.util.SerializableMap<java.lang.String, java.lang.String> org.apache.iceberg.io.StorageCredential::config()

[danielcweeks]

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.

[danielcweeks]

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.

[nastra]

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 (which isn't configurable and is generally what you want) 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 (EsotericSoftware/kryo#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.

[ajantha-bhat]

@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.

[nastra]

I don't think introducing this helps either, because it also introduces breaking API changes

[ajantha-bhat]

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

[nastra]

FYI I've opened #12930 as an alternative to this so that we can try and avoid the API breakage while still making ser/de properly work with Kryo

[ajantha-bhat]

closing in the favor of #12930