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

TEPHRA-171 Added implementation TephraZKClientService which does not depend on the guava service listeners. #115

Merged
merged 1 commit into from Feb 18, 2016

Conversation

sagarkapare
Copy link
Contributor

@@ -29,7 +29,6 @@
import com.google.inject.Guice;
import com.google.inject.Injector;
import org.apache.hadoop.conf.Configuration;
import org.apache.twill.zookeeper.ZKClientService;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all occurrences of Twill's ZKClientService from tephra.


@Override
public void startAndWait() {
((ServiceDelegate) serviceDelegate).connectToZookeeper();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicitly calling connectToZookeeper to synchronously connecting to the zookeeper. This may not be the best method since we require casting here.

@sagarkapare
Copy link
Contributor Author

@chtyim @poornachandra Please take a look. Thanks!

/**
* Initiates service startup (if necessary), returning once the service has finished starting.
*/
void startAndWait();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just call it start and stop.

import java.util.List;

/**
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a line of javadoc.

@chtyim
Copy link
Contributor

chtyim commented Feb 18, 2016

Just one comment. Otherwise the code change LGTM. Also make sure we don't use ZKClientService.Builder in the rest of the code base.

@sagarkapare
Copy link
Contributor Author

Thank you @chtyim for the review. Made sure we are not using ZKClientService.Builder in tephra. Added javadoc and also removed the unused constructor from the TephraZKClientService. Will merge once the build passes.

@sagarkapare
Copy link
Contributor Author

Build passed. Merging.

sagarkapare added a commit that referenced this pull request Feb 18, 2016
TEPHRA-171 Added ZkClientService in Tephra which does not extend from…
@sagarkapare sagarkapare merged commit c20ec70 into develop Feb 18, 2016
@sagarkapare sagarkapare deleted the fix/copy-zk branch February 18, 2016 21:29
@sagarkapare sagarkapare changed the title TEPHRA-171 Added ZkClientService in Tephra which does not extend from… TEPHRA-171 Added implementation TephraZKClientService which does not depend on the guava service listeners. Feb 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants