API, Core: Move @Value.Immutable usage from iceberg-api to iceberg-core #8099
Conversation
nastra
force-pushed
the
move-immutables-to-core
branch
from
f3440f9 to
4f0d49d
Compare
|
+1, make sense. API module should only include interface contracts and ideally zero dependency on anything else. |
nastra
force-pushed
the
move-immutables-to-core
branch
from
4f0d49d to
5eb28f8
Compare
| // any Immutable-specific class to the classpath | ||
| @SuppressWarnings("ImmutablesStyle") | ||
| @Value.Style(typeImmutableEnclosing = "ImmutableSnapshotTable") | ||
| public interface BaseSnapshotTable extends SnapshotTable { |
There was a problem hiding this comment.
I forgot to check this in the last PR, but I really don't like that this is a public interface that completely duplicates the SnapshotTable interface from API. Is it possible to make this package-private? Or is that a limitation of Immutables?
There was a problem hiding this comment.
yes that's absolutely possible. Just worth mentioning that the generated class ImmutableSnapshotTable will have the same visibility, unless we add visibility = Value.Style.ImplementationVisibility.PUBLIC to make the generated class public. Did you want to only make this one package-private?
There was a problem hiding this comment.
Yes, definitely. We want to make sure that people don't see or use these because they aren't real interfaces. They're just artifacts of using Immutables.
Can you also fix the other PR if those were public?
There was a problem hiding this comment.
it turns out adding visibility = Value.Style.ImplementationVisibility.PUBLIC will cause warnings for consuming clients (same issue as #291) and the suggested workaround isn't really feasible for us.
I've opened immutables/immutables#1474 to fix this in Immutables and verified that consuming clients don't see the below warning with my proposed fix
warning: unknown enum constant ImplementationVisibility.PUBLIC
reason: class file for org.immutables.value.Value$Style$ImplementationVisibility not found
Should we go ahead and merge this PR for now to unblock other work and I'll follow up and make those classes package-private but their generated classes public once the issue is fixed upstream?
There was a problem hiding this comment.
What about just copying the Immutable output classes into src? Then we don't need the duplicate interfaces or codegen not doing what we want.
There was a problem hiding this comment.
I've looked closer into this and I don't think this is actually a direct issue of using Immutables per-se, but rather something with javac. The generated class looks identical with/without the ImplementationVisibility and the javac warning won't cause any issues for consuming clients.
I went ahead an made all relevant interfaces package-protected and specified the visibility of the generated Class/Builder.
nastra
force-pushed
the
move-immutables-to-core
branch
from
5eb28f8 to
4193b7d
Compare
nastra
force-pushed
the
move-immutables-to-core
branch
from
4193b7d to
f7ef545
Compare
|
Thanks, @nastra! I'm glad you were able to solve the immutables problem. |
3 participants
This moves usage of
@Value.Immutableon all Actions Result classes toiceberg-coreby maintaining the original naming of the classes and builders. Moving this will not cause breaking API changes for consuming users, because most (all?) users will have a dependency toiceberg-corewhen usingiceberg-api.The reason for moving this from
iceberg-apitoiceberg-coreis because we don't want to make generated Builders from@Value.Immutablepart of the API module and the respective API guarantees.View-specific immutable classes are moved as part of #7992.
Once both PRs are merged, I'll add a follow-up and mention in the docs that
@Value.Immutableusage is discouraged oniceberg-apiand will also remove the immutable dependency for that project