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.

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

Support Caching module for iceberg connector #16942

Merged
merged 1 commit into from Nov 5, 2021
Merged

Conversation

maobaolong
Copy link
Contributor

@maobaolong maobaolong commented Nov 3, 2021

Fix #16925

Test plan - (Please fill in how you tested your changes)

Using iceberg catalog to create an iceberg table with the following iceberg.properties.

connector.name=iceberg
hive.metastore.uri=thrift://localhost:9083
cache.enabled=true
cache.type=ALLUXIO
cache.base-directory=file:///tmp/alluxio
cache.alluxio.max-cache-size=1GB
hive.node-selection-strategy=SOFT_AFFINITY
hive.config.resources=/softwares/hadoop-2.8.5/etc/hadoop/core-site.xml

The presto cannot start successful without this PR.

== RELEASE NOTES ==

Iceberg Changes
* Support Caching module for iceberg connector.

@linux-foundation-easycla
Copy link

@linux-foundation-easycla linux-foundation-easycla bot commented Nov 3, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Member

@beinan beinan left a comment

I'm so excited to see local cache is able to work in our iceberg connector. Thank you so much! @maobaolong!.

But looks like your change breaks the test.

Error:  init(com.facebook.presto.iceberg.TestIcebergDistributed)  Time elapsed: 15.404 s  <<< FAILURE!
com.google.inject.CreationException: 
Unable to create injector, see the following errors:

1) A binding to com.facebook.presto.cache.CacheManager was already configured at com.facebook.presto.iceberg.IcebergModule.createCacheManager().
  at com.facebook.presto.cache.CachingModule.createCacheManager(CachingModule.java:57)

1 error
	at com.google.inject.internal.Errors.throwCreationExceptionIfErrorsExist(Errors.java:543)
	at com.google.inject.internal.InternalInjectorCreator.initializeStatically(InternalInjectorCreator.java:159)
	at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:106)
	at com.google.inject.Guice.createInjector(Guice.java:87)
	at com.facebook.airlift.bootstrap.Bootstrap.initialize(Bootstrap.java:251)
	at com.facebook.presto.iceberg.InternalIcebergConnectorFactory.createConnector(InternalIcebergConnectorFactory.java:80)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.facebook.presto.iceberg.IcebergConnectorFactory.create(IcebergConnectorFactory.java:49)
	at com.facebook.presto.connector.ConnectorManager.createConnector(ConnectorManager.java:379)
	at com.facebook.presto.connector.ConnectorManager.addCatalogConnector(ConnectorManager.java:231)
	at com.facebook.presto.connector.ConnectorManager.createConnection(ConnectorManager.java:223)
	at com.facebook.presto.connector.ConnectorManager.createConnection(ConnectorManager.java:209)
	at com.facebook.presto.server.testing.TestingPrestoServer.createCatalog(TestingPrestoServer.java:477)
	at com.facebook.presto.tests.DistributedQueryRunner.createCatalog(DistributedQueryRunner.java:459)
	at com.facebook.presto.iceberg.IcebergQueryRunner.createIcebergQueryRunner(IcebergQueryRunner.java:96)
	at com.facebook.presto.iceberg.IcebergQueryRunner.createIcebergQueryRunner(IcebergQueryRunner.java:56)
	at com.facebook.presto.iceberg.IcebergQueryRunner.createIcebergQueryRunner(IcebergQueryRunner.java:42)
	at com.facebook.presto.iceberg.TestIcebergDistributed.createQueryRunner(TestIcebergDistributed.java:34)
	at com.facebook.presto.tests.AbstractTestQueryFramework.init(AbstractTestQueryFramework.java:82)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:515)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:217)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:144)
	at org.testng.internal.TestMethodWorker.invokeBeforeClassMethods(TestMethodWorker.java:169)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Error:  init(com.facebook.presto.iceberg.TestIcebergDistributedNative)  Time elapsed: 15.404 s  <<< FAILURE!
com.google.inject.CreationException: 
Unable to create injector, see the following errors:

1) A binding to com.facebook.presto.cache.CacheManager was already configured at com.facebook.presto.iceberg.IcebergModule.createCacheManager().
  at com.facebook.presto.cache.CachingModule.createCacheManager(CachingModule.java:57)

1 error
	at com.google.inject.internal.Errors.throwCreationExceptionIfErrorsExist(Errors.java:543)
	at com.google.inject.internal.InternalInjectorCreator.initializeStatically(InternalInjectorCreator.java:159)
	at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:106)
	at com.google.inject.Guice.createInjector(Guice.java:87)
	at com.facebook.airlift.bootstrap.Bootstrap.initialize(Bootstrap.java:251)
	at com.facebook.presto.iceberg.InternalIcebergConnectorFactory.createConnector(InternalIcebergConnectorFactory.java:80)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.facebook.presto.iceberg.IcebergConnectorFactory.create(IcebergConnectorFactory.java:49)
	at com.facebook.presto.connector.ConnectorManager.createConnector(ConnectorManager.java:379)
	at com.facebook.presto.connector.ConnectorManager.addCatalogConnector(ConnectorManager.java:231)
	at com.facebook.presto.connector.ConnectorManager.createConnection(ConnectorManager.java:223)
	at com.facebook.presto.connector.ConnectorManager.createConnection(ConnectorManager.java:209)
	at com.facebook.presto.server.testing.TestingPrestoServer.createCatalog(TestingPrestoServer.java:477)
	at com.facebook.presto.tests.DistributedQueryRunner.createCatalog(DistributedQueryRunner.java:459)
	at com.facebook.presto.iceberg.IcebergQueryRunner.createIcebergQueryRunner(IcebergQueryRunner.java:96)
	at com.facebook.presto.iceberg.IcebergQueryRunner.createIcebergQueryRunner(IcebergQueryRunner.java:56)
	at com.facebook.presto.iceberg.IcebergQueryRunner.createNativeIcebergQueryRunner(IcebergQueryRunner.java:47)
	at com.facebook.presto.iceberg.TestIcebergDistributedNative.createQueryRunner(TestIcebergDistributedNative.java:35)
	at com.facebook.presto.tests.AbstractTestQueryFramework.init(AbstractTestQueryFramework.java:82)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:515)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:217)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:144)
	at org.testng.internal.TestMethodWorker.invokeBeforeClassMethods(TestMethodWorker.java:169)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Error:  tearDown(com.facebook.presto.iceberg.TestIcebergMetadataListing)  Time elapsed: 0.002 s  <<< FAILURE!
java.lang.NullPointerException
	at com.facebook.presto.tests.AbstractTestQueryFramework.getSession(AbstractTestQueryFramework.java:108)
	at com.facebook.presto.tests.AbstractTestQueryFramework.assertQuerySucceeds(AbstractTestQueryFramework.java:225)
	at com.facebook.presto.iceberg.TestIcebergMetadataListing.tearDown(TestIcebergMetadataListing.java:87)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:515)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:217)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:144)
	at org.testng.internal.TestMethodWorker.invokeAfterClassMethods(TestMethodWorker.java:217)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:115)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Error:  tearDown(com.facebook.presto.iceberg.TestIcebergSystemTables)  Time elapsed: 0 s  <<< FAILURE!
java.lang.NullPointerException
	at com.facebook.presto.tests.AbstractTestQueryFramework.getSession(AbstractTestQueryFramework.java:108)
	at com.facebook.presto.tests.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:195)
	at com.facebook.presto.iceberg.TestIcebergSystemTables.tearDown(TestIcebergSystemTables.java:178)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
	at org.testng.internal.Invoker.invokeConfigurationMethod(Invoker.java:515)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:217)
	at org.testng.internal.Invoker.invokeConfigurations(Invoker.java:144)
	at org.testng.internal.TestMethodWorker.invokeAfterClassMethods(TestMethodWorker.java:217)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:115)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

2021-11-03T10:18:42.747-0600 INFO Life cycle stopping...
2021-11-03T10:18:42.750-0600 INFO Life cycle stopping...
2021-11-03T10:18:42.771-0600 INFO Life cycle stopping...
2021-11-03T10:18:42.773-0600 INFO Life cycle stopping...
2021-11-03T10:18:42.774-0600 INFO Life cycle stopping...
2021-11-03T10:18:42.777-0600 INFO Life cycle stopping...
[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    TestIcebergDistributed>AbstractTestQueryFramework.init:82->createQueryRunner:34 » Creation
Error:    TestIcebergDistributedNative>AbstractTestQueryFramework.init:82->createQueryRunner:35 » Creation
Error:    TestIcebergMetadataListing.tearDown:87->AbstractTestQueryFramework.assertQuerySucceeds:225->AbstractTestQueryFramework.getSession:108 » NullPointer
Error:    TestIcebergSystemTables.tearDown:178->AbstractTestQueryFramework.assertUpdate:195->AbstractTestQueryFramework.getSession:108 » NullPointer
[INFO] 
Error:  Tests run: 862, Failures: 4, Errors: 0, Skipped: 851

Copy link
Member

@beinan beinan left a comment

lgtm, @ChunxuTang would you like to take a second look? Thanks!

return ImmutableList.of(
sortedCandidates.get(position),
sortedCandidates.get((position + 1) % size));
}
return addresses;
Copy link
Member

@beinan beinan Nov 4, 2021

Choose a reason for hiding this comment

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

I guess these lines are copied from HiveSplit, can we extract these lines into a util method? then we can share the implementation. But I'm fine if you or someone else would like to do it in another PR, your call.

Copy link
Contributor Author

@maobaolong maobaolong Nov 4, 2021

Choose a reason for hiding this comment

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

Yeah, it copied from HiveSplit, i try to abstract a common method to place this logic to a util class, but I cannot decide whether we could add a new dependency guava to the presto-common, presto-spi or some other module. As I'm not familiar enough with presto now, it would be nice to do this method abstraction in another PR, thank you.

@maobaolong
Copy link
Contributor Author

@maobaolong maobaolong commented Nov 4, 2021

@beinan Thanks for your review and reply.

@ChunxuTang
Copy link
Contributor

@ChunxuTang ChunxuTang commented Nov 4, 2021

LGTM. Thanks for your nice work! @maobaolong
Just a minor suggestion: Could you squash your commits into one?

@maobaolong @beinan May I expect in the future you folks can share some evaluation results on benchmarks with this caching feature? I believe this feature will help a lot with improving query performance on Iceberg tables.

beinan
beinan approved these changes Nov 4, 2021
Copy link
Member

@beinan beinan left a comment

Hey @maobaolong , the code change looks good to me, could you please rebase/squash the 3 commits into 1 commit? also the commit message should follow the pattern below:

Fix OOM caused by foo in bar

Foo was pack ratting ByteBuffers causing OOM.

Cherry-pick of https://github.com/foo/bar/pull/123 (https://github.com/foo/bar/pull/123)

Co-authored-by: Foo Bar <foo@bar.com>

Again, really appreciate the contribution!

@maobaolong
Copy link
Contributor Author

@maobaolong maobaolong commented Nov 5, 2021

@beinan @ChunxuTang Sorry I just use the following command to squash commits into one

git rebase -i HEAD~3

and use fixup to squash commits, is this ok for you? Please feel free to let me fix again if you think the comment is not good enough.

@ChunxuTang
Copy link
Contributor

@ChunxuTang ChunxuTang commented Nov 5, 2021

@maobaolong
Yep~ It looks good to me. Looks like this is not a cherry-pick commit, thus the commit message also looks good to me. If this is a cherry-pick commit, then you may want to follow the commit message pattern @beinan shows here.
Thanks again for your work!

@beinan beinan merged commit d7ecb91 into prestodb:master Nov 5, 2021
40 checks passed
@zhengxingmao
Copy link
Contributor

@zhengxingmao zhengxingmao commented Nov 23, 2021

Met this error when enable cache for iceberg connector .
java.lang.UnsupportedOperationException: s3a is not supported as the external filesystem.
at alluxio.hadoop.LocalCacheFileSystem.initialize(LocalCacheFileSystem.java:91)
at com.facebook.presto.cache.alluxio.AlluxioCachingFileSystem.initialize(AlluxioCachingFileSystem.java:72)
at com.facebook.presto.cache.CacheFactory.createCachingFileSystem(CacheFactory.java:54)
at com.facebook.presto.hive.cache.HiveCachingHdfsConfiguration.lambda$getConfiguration$0(HiveCachingHdfsConfiguration.java:71)
at com.facebook.presto.hive.cache.HiveCachingHdfsConfiguration$CachingJobConf.createFileSystem(HiveCachingHdfsConfiguration.java:105)
at org.apache.hadoop.fs.PrestoFileSystemCache.get(PrestoFileSystemCache.java:59)
at org.apache.hadoop.fs.FileSystem.get(FileSystem.java:373)
at org.apache.hadoop.fs.Path.getFileSystem(Path.java:295)
at com.facebook.presto.hive.HdfsEnvironment.lambda$getFileSystem$0(HdfsEnvironment.java:71)
at com.facebook.presto.hive.authentication.NoHdfsAuthentication.doAs(NoHdfsAuthentication.java:23)
at com.facebook.presto.hive.HdfsEnvironment.getFileSystem(HdfsEnvironment.java:70)
at com.facebook.presto.hive.HdfsEnvironment.getFileSystem(HdfsEnvironment.java:64)

@varungajjala varungajjala mentioned this pull request Nov 23, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants