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

Rubix integration #2679

Merged
merged 1 commit into from Mar 18, 2020
Merged

Rubix integration #2679

merged 1 commit into from Mar 18, 2020

Conversation

@stagraqubole
Copy link
Member

stagraqubole commented Jan 30, 2020

RubixConfigurationInitializer is like other ConfigurationInitializers, it sets Rubix configs into Configuration object

RubixInitializer initializes RubixConfigurationInitializer. It waits for the node to join the cluster and update RubixConfigurationInitializer with info like master address that will be
needed to be placed into Configuration objects by RubixConfigurationInitializer.

Flow of events:
Presto server is started
	RubixInitializer waits on the node to join cluster
	On joining:
		It initializes RubixConfigurationInitializer
		It also creates the two servers needed by Rubix: BookkeeperServer and LocalDataTransferServer
		Finally, the BookKeeper object is registered with CachingFileSystem to be used for direct communication with BookkeeperServer instead of thrift 
		It informs RubixConfigurationInitializer of initialization complete so that RubixConfigurationInitializer can enable Rubix in Configuration objects from now on
@cla-bot cla-bot bot added the cla-signed label Jan 30, 2020
@stagraqubole stagraqubole requested a review from electrum Jan 30, 2020
@findepi findepi changed the title Rubix integration (wip) Rubix integration Jan 31, 2020
@findepi findepi added the WIP label Jan 31, 2020
@stagraqubole stagraqubole force-pushed the stagraqubole:rubix-master branch 2 times, most recently from 0350eae to 8b40b04 Feb 3, 2020
@dain dain changed the title Rubix integration [WIP] Rubix integration Feb 3, 2020
ExecutorService initializerService = Executors.newSingleThreadExecutor();
ListenableFuture<Boolean> nodeJoinFuture = MoreExecutors.listeningDecorator(initializerService).submit(() ->
{
while (!(nodeManager.getAllNodes().contains(nodeManager.getCurrentNode()) &&

This comment has been minimized.

Copy link
@raunaqmorarka

raunaqmorarka Feb 3, 2020

Member

I think nodeManager.getAllNodes() should be called only once per loop here

ListenableFuture<Boolean> nodeJoinFuture = MoreExecutors.listeningDecorator(initializerService).submit(() ->
{
while (!(nodeManager.getAllNodes().contains(nodeManager.getCurrentNode()) &&
nodeManager.getAllNodes().stream().anyMatch(Node::isCoordinator))) {

This comment has been minimized.

Copy link
@raunaqmorarka

raunaqmorarka Feb 3, 2020

Member

we could use !allNodes.getActiveCoordinators().isEmpty() here

This comment has been minimized.

Copy link
@stagraqubole

stagraqubole Mar 2, 2020

Author Member

nodeManager in an instance of NodeManager which doesnt have AllNodes methods (which is in InternalNodeManager) so this call is not possible

ExecutorService initializerService = Executors.newSingleThreadExecutor();
ListenableFuture<Boolean> nodeJoinFuture = MoreExecutors.listeningDecorator(initializerService).submit(() ->
{
while (!(nodeManager.getAllNodes().contains(nodeManager.getCurrentNode()) &&

This comment has been minimized.

Copy link
@raunaqmorarka

raunaqmorarka Feb 3, 2020

Member

Can we do this via InternalNodeManager#addNodeChangeListener rather than the looping here ?

This comment has been minimized.

Copy link
@stagraqubole

stagraqubole Mar 2, 2020

Author Member

Yes, It would be cleaner.
It will need changes to NodeManager interface and implementations to work with InternalNodeManager's listener methods. Handling removeNodeChangeListener in NodeManager could be tricky since callers are not aware of AllNodes class.

@stagraqubole stagraqubole force-pushed the stagraqubole:rubix-master branch from 8b40b04 to f49b49d Feb 5, 2020
@stagraqubole stagraqubole force-pushed the stagraqubole:rubix-master branch from 6afbff9 to e73891b Feb 14, 2020
@stagraqubole stagraqubole force-pushed the stagraqubole:rubix-master branch from e73891b to de48021 Mar 2, 2020
@raunaqmorarka

This comment has been minimized.

Copy link
Member

raunaqmorarka commented Mar 5, 2020

@stagraqubole please check if HiveWriteUtils#isS3FileSystem needs to be updated. I think we would want it to behave the same as S3 even with Rubix FS.

@stagraqubole stagraqubole force-pushed the stagraqubole:rubix-master branch from de48021 to 1769b1f Mar 6, 2020
@stagraqubole

This comment has been minimized.

Copy link
Member Author

stagraqubole commented Mar 6, 2020

@stagraqubole please check if HiveWriteUtils#isS3FileSystem needs to be updated. I think we would want it to behave the same as S3 even with Rubix FS.

Thanks, missed this. Have added a check now.

config.set("fs.s3.impl", RUBIX_S3_FS_CLASS_NAME);
config.set("fs.s3a.impl", RUBIX_S3_FS_CLASS_NAME);
config.set("fs.s3n.impl", RUBIX_S3_FS_CLASS_NAME);
config.set("fs.wasb.impl", RUBIX_AZURE_FS_CLASS_NAME);

This comment has been minimized.

Copy link
@raunaqmorarka

raunaqmorarka Mar 6, 2020

Member

fs.wasbs.impl missing here ?

@raunaqmorarka

This comment has been minimized.

Copy link
Member

raunaqmorarka commented Mar 6, 2020

It will be good to have some sort of integration test in this repo. Maybe the Rubix equivalent of test-hive-hadoop2-s3 profile can be implemented and run as part of product tests.

@dain dain changed the title [WIP] Rubix integration Rubix integration Mar 18, 2020
@dain dain removed the WIP label Mar 18, 2020
@dain
dain approved these changes Mar 18, 2020
Copy link
Member

dain left a comment

This is good enough for a new feature that is disabled by default. I will put a few patches after merging this version

@dain dain merged commit 540e14c into prestosql:master Mar 18, 2020
34 checks passed
34 checks passed
maven-checks
Details
hive-tests (config-empty)
Details
x (config-empty, suite-1)
Details
hive-tests (config-hdp3)
Details
x (config-empty, suite-2)
Details
x (config-empty, suite-3)
Details
x (config-empty, suite-5)
Details
x (config-empty, suite-6-non-generic)
Details
x (config-empty, suite-7-non-generic)
Details
x (config-hdp3, suite-1)
Details
x (config-hdp3, suite-2)
Details
x (config-hdp3, suite-3)
Details
x (config-hdp3, suite-5)
Details
x (config-cdh5, suite-1)
Details
x (config-cdh5, suite-2)
Details
x (config-cdh5, suite-3)
Details
x (config-cdh5, suite-5)
Details
error-prone-checks
Details
kudu-tests
Details
web-ui-checks
Details
test-other-modules
Details
x (presto-main)
Details
x (presto-tests) x (presto-tests)
Details
x (presto-tests -P ci-only)
Details
x (presto-raptor-legacy)
Details
x (presto-accumulo)
Details
x (presto-cassandra)
Details
x (presto-hive,presto-orc)
Details
x (presto-hive,presto-parquet -P test-parquet)
Details
x (presto-mongodb,presto-kafka,presto-elasticsearch)
Details
x (presto-redis)
Details
x (presto-sqlserver,presto-postgresql,presto-mysql)
Details
x (presto-phoenix,presto-iceberg)
Details
verification/cla-signed
Details
@dain dain mentioned this pull request Apr 6, 2020
7 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.