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

OMID-202 Refactor Omid to use Netty 4 #92

Closed
wants to merge 1 commit into from
Closed

Conversation

stoty
Copy link
Contributor

@stoty stoty commented Mar 17, 2021

No description provided.

@@ -342,7 +344,7 @@ public void nodeChanged() throws Exception {
setTSOAddress(hp.getHost(), hp.getPort());
epoch = Long.parseLong(currentTSOAndEpochArray[1]);
LOG.info("CurrentTSO ZNode changed. New TSO Host & Port {}/Epoch {}", hp, getEpoch());
if (currentChannel != null && currentChannel.isConnected()) {
if (currentChannel != null && currentChannel.isActive()) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to consolidate this into a helper since you're modifying it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which part do you mean ?
This handles the case when the active server changes in ZK, and AFAICT this is the only place where it is handled.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I was suggesting to consolidate these two things into one method, aka boolean isChannelActive(Channel) { return channel != null && channel.isActive();}

Can ignore if you don't think there's value in it :)

Copy link
Member

@joshelser joshelser left a comment

Choose a reason for hiding this comment

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

Thanks for replying to my feedback! looks great.

@stoty stoty closed this Mar 24, 2021
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