Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Core, API: Move SQLViewRepresentation to API #9278

Closed
wants to merge 1 commit into from
Closed

Core, API: Move SQLViewRepresentation to API #9278

wants to merge 1 commit into from

Conversation

amogh-jahagirdar
Copy link
Contributor

This change is similar to the one done a while back for BaseViewVersion. SQLViewRepresentation is moved from the core module to the API module (since it is a concept that's apparent in the spec). This stems from this thread https://github.com/apache/iceberg/pull/9247/files#r1419759456

amogh-jahagirdar
amogh-jahagirdar commented Dec 11, 2023
View reviewed changes
core/src/main/java/org/apache/iceberg/view/BaseSQLViewRepresentation.java Outdated
Comment on lines 25 to 29
@Value.Style(
typeImmutable = "ImmutableSQLViewRepresentation",
visibilityString = "PUBLIC",
builderVisibilityString = "PUBLIC")
interface BaseSQLViewRepresentation extends SQLViewRepresentation {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package private BaseSQLViewRepresentation which extends the public SQLViewRepresentation. The generated Immutables class is public for preserving compatibility.

amogh-jahagirdar
amogh-jahagirdar commented Dec 11, 2023
View reviewed changes
.palantir/revapi.yml Outdated
Comment on lines 880 to 883
- code: "java.method.visibilityReduced"
old: "method org.apache.iceberg.view.ImmutableSQLViewRepresentation org.apache.iceberg.view.ImmutableSQLViewRepresentation::copyOf(org.apache.iceberg.view.SQLViewRepresentation)"
new: "method org.apache.iceberg.view.ImmutableSQLViewRepresentation org.apache.iceberg.view.ImmutableSQLViewRepresentation::copyOf(org.apache.iceberg.view.BaseSQLViewRepresentation)"
justification: "Immutables generated copyOf visibility changed"
Copy link
Contributor Author

@amogh-jahagirdar amogh-jahagirdar Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there must have been some behavior change in Immutables between the original code generation for SQLViewRepresentation and now. Immutables now generates a package-private copyOf (confirmed with ViewVersion). However, prior to this change the generated copyOf for SQLViewRepresentation was actually public. This is technically breaking to downgrade the copyOf but I don't see any options in the Immutables docs to control this behavior. Before merging this though, I'll explore a few options. Since the view APIs are newer we have more flexibility here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately it looks like the visibility of copyOf() is tied directly to the visibility of BaseSQLViewRepresentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it's tied to the visibility of the actual class which makes sense. Since we probably want to just go for 100% backwards compatibility I went with maintaining a copy of the Immutables generated class. I explored the option of generating a package private immutable class which we extend but Inheritance of the class isn't possible since it's final (Immutables only generates final classes as far as I can tell)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to exlore some other alternatives this week

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amogh-jahagirdar I've created #9302 which doesn't break the API and also doesn't require to copy anything from the generated class

@amogh-jahagirdar
Copy link
Contributor Author

Looks like some unrelated Flink tests failed, noting here:


TestIcebergSourceWithWatermarkExtractor > testThrottling FAILED
    java.lang.AssertionError: 
    Expecting
      <CompletableFuture[Failed with the following stack trace:
    java.lang.IllegalStateException: Trying to access closed classloader. Please check if you store classloaders directly or indirectly in static fields. If the stacktrace suggests that the leak occurs in a third party library and cannot be fixed immediately, you can disable this check with the configuration 'classloader.check-leaked-classloader'.
    	at org.apache.flink.util.FlinkUserCodeClassLoaders$SafetyNetWrapperClassLoader.ensureInner(FlinkUserCodeClassLoaders.java:184)
    	at org.apache.flink.util.FlinkUserCodeClassLoaders$SafetyNetWrapperClassLoader.loadClass(FlinkUserCodeClassLoaders.java:192)
    	at java.lang.Class.forName0(Native Method)
    	at java.lang.Class.forName(Class.java:348)
    	at com.esotericsoftware.kryo.util.DefaultClassResolver.readName(DefaultClassResolver.java:136)
    	at com.esotericsoftware.kryo.util.DefaultClassResolver.readClass(DefaultClassResolver.java:115)
    	at com.esotericsoftware.kryo.Kryo.readClass(Kryo.java:641)
    	at com.esotericsoftware.kryo.Kryo.readClassAndObject(Kryo.java:752)
    	at org.apache.flink.api.java.typeutils.runtime.kryo.KryoSerializer.deserialize(KryoSerializer.java:403)
    	at org.apache.flink.streaming.api.operators.collect.CollectCoordinationResponse.getResults(CollectCoordinationResponse.java:84)
    	at org.apache.flink.streaming.api.operators.collect.AbstractCollectResultBuffer.addResults(AbstractCollectResultBuffer.java:127)
    	at org.apache.flink.streaming.api.operators.collect.AbstractCollectResultBuffer.dealWithResponse(AbstractCollectResultBuffer.java:91)
    	at org.apache.flink.streaming.api.operators.collect.CollectResultFetcher.next(CollectResultFetcher.java:148)
    	at org.apache.flink.streaming.api.operators.collect.CollectResultIterator.nextResultFromFetcher(CollectResultIterator.java:106)
    	at org.apache.flink.streaming.api.operators.collect.CollectResultIterator.hasNext(CollectResultIterator.java:80)
    	at org.apache.iceberg.flink.source.TestIcebergSourceWithWatermarkExtractor.lambda$waitForRecords$6(TestIcebergSourceWithWatermarkExtractor.java:409)
    	at java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1604)
    	at java.util.concurrent.CompletableFuture$AsyncSupply.exec(CompletableFuture.java:1596)
    	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
    	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
    	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
    	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:175)

@amogh-jahagirdar
Copy link
Contributor Author

Ah I see the flink failure was fixed recently in #9216, I just need to rebase

amogh-jahagirdar
amogh-jahagirdar commented Dec 13, 2023
View reviewed changes
core/src/main/java/org/apache/iceberg/view/ImmutableSQLViewRepresentation.java
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;

/** This is a copy of the Immutables generated class to preserve backwards compatibility */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep full backwards compatibility the approach taken here is to keep a copy of the Immutables generated code which is unfortunate; but preserving backwards compatibility is probably more important. I had to edit the class to make it respect our checkstyle.

@rdblue
Copy link
Contributor

rdblue commented Dec 14, 2023

Closing in favor of #9302

@rdblue rdblue closed this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdblue rdblue Awaiting requested review from rdblue

@nastra nastra Awaiting requested review from nastra

Assignees
No one assigned
Labels
API core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amogh-jahagirdar @rdblue @nastra