Skip to content

Add new interface IZkStateListener to provide session aware handleNewSession for ZkHelixManager#644

Merged
jiajunwang merged 22 commits intoapache:masterfrom
huizhilu:IZkStateListener
Dec 16, 2019
Merged

Add new interface IZkStateListener to provide session aware handleNewSession for ZkHelixManager#644
jiajunwang merged 22 commits intoapache:masterfrom
huizhilu:IZkStateListener

Conversation

@huizhilu
Copy link
Contributor

@huizhilu huizhilu commented Dec 4, 2019

Issues

  • My PR addresses the following Helix issues and references them in the PR description:

Fixes #643

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Current I0ITec IZkStateListener doesn't have an API handleNewSession(sessionId), which is needed for #642
To avoid creating a new stateListener extends IZkStateListener, it seems better to move IZkStateListener to helix internal. So we would have flexible control to add the new API.

Changelist:

  1. Add new IZkStateListener to helix. The new IZkStateListener has new api handleNewSession(sessionId), and removes handleNewSession()
  2. Add tests for this new IZkStateListener.
  3. Add session id to ZkEvent as a field.
  4. For state change events, pass session id in zk events.

Tests

  • The following tests are written for this issue:

  • testSubscribeStateChanges()

  • testSessionExpiry()

  • testSubscribeStateChangesForI0ItecIZkStateListener()

  • testSessionExpiryForI0IItecZkStateListener()

  • The following is the result of the "mvn test" command on the appropriate module:

[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestJobQueueCleanUp.testJobQueueAutoCleanUp » ThreadTimeout Method org.testng....
[ERROR] TestTaskRebalancerStopResume.stopDeleteJobAndResumeNamedQueue:199 » Helix Fail...
[INFO]
[ERROR] Tests run: 887, Failures: 2, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:00 h
[INFO] Finished at: 2019-12-04T12:38:29-08:00
[INFO] ------------------------------------------------------------------------

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.316 s - in org.apache.helix.integration.task.TestJobQueueCleanUp
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 21.841 s - in org.apache.helix.integration.task.TestTaskRebalancerStopResume
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0

Commits

  • My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Code Quality

  • My diff has been formatted using helix-style.xml

@huizhilu huizhilu requested a review from jiajunwang December 4, 2019 22:52
@huizhilu huizhilu closed this Dec 5, 2019
@huizhilu huizhilu reopened this Dec 5, 2019
@huizhilu huizhilu changed the title Customize IZkStateListener to add handleNewSession(sessionId) Add new interface IZkStateListener to provide session aware handleNewSession for ZkHelixManager Dec 6, 2019
Copy link
Contributor

@narendly narendly left a comment

Choose a reason for hiding this comment

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

Overall looks okay to me. But let's discuss if removing public methods would be okay. I think ZkCache is an internal concept that users shouldn't be using for their own logic, but let's double check.

Also please address other comments.

Copy link
Contributor Author

@huizhilu huizhilu left a comment

Choose a reason for hiding this comment

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

Thanks, @narendly, for the review and approval. I will address the comments.

@huizhilu
Copy link
Contributor Author

huizhilu commented Dec 9, 2019

Rerun tests. All passed.

[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestRenamePartition.testRenamePartitionCustomIS:124 expected:true but was:false
[ERROR] TestDeleteJobFromJobQueue.testForceDeleteJobFromJobQueue:75 » Helix Failed to ...
[INFO]
[ERROR] Tests run: 890, Failures: 2, Errors: 0, Skipped: 0

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 8.371 s - in org.apache.helix.integration.TestRenamePartition
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS

[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 7.033 s - in org.apache.helix.integration.task.TestDeleteJobFromJobQueue
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS

@huizhilu
Copy link
Contributor Author

Tests passed.

[INFO] Tests run: 890, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3,384.407 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 890, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 56:29 min
[INFO] Finished at: 2019-12-11T16:44:46-08:00

This PR is ready to be merged, approved by @jiajunwang @narendly

Commit message:

I0Itec IZkStateListener doesn't have an API handleNewSession(sessionId) to handle session aware operation,
which is needed to fix session race condition for creating ephemeral node in ZkClient.
So this new IZkStateListener interface is introduced to provide session aware handleNewSession method for ZkHelixManager.

Changelist:
- Introduce new IZkStateListener to helix. The new IZkStateListener adds new method
handleNewSession(String sessionId), and removes the old method handleNewSession().
- Add default implementations I0ItecIZkStateListenerHelixImpl in IZkStateListener
and IZkStateListenerI0ItecImpl in ZkClient for backward compatibility.
- Add session id to ZkEvent as a private field to help debug ZkEvent.
- Add unit tests to test subscribe/unsubscribe state changes.

@jiajunwang jiajunwang merged commit 660ae7c into apache:master Dec 16, 2019
narendly pushed a commit to narendly/helix that referenced this pull request Dec 24, 2019
…Session for ZkHelixManager (apache#644)

I0Itec IZkStateListener doesn't have an API handleNewSession(sessionId) to handle session aware operation,
which is needed to fix session race condition for creating ephemeral node in ZkClient.
So this new IZkStateListener interface is introduced to provide session aware handleNewSession method for ZkHelixManager.

Changelist:
- Introduce new IZkStateListener to helix. The new IZkStateListener adds new method
handleNewSession(String sessionId), and removes the old method handleNewSession().
- Add default implementations I0ItecIZkStateListenerHelixImpl in IZkStateListener
and IZkStateListenerI0ItecImpl in ZkClient for backward compatibility.
- Add session id to ZkEvent as a private field to help debug ZkEvent.
- Add unit tests to test subscribe/unsubscribe state changes.
zhangmeng916 pushed a commit to zhangmeng916/helix that referenced this pull request Feb 7, 2020
…Session for ZkHelixManager (apache#644)

I0Itec IZkStateListener doesn't have an API handleNewSession(sessionId) to handle session aware operation,
which is needed to fix session race condition for creating ephemeral node in ZkClient.
So this new IZkStateListener interface is introduced to provide session aware handleNewSession method for ZkHelixManager.

Changelist:
- Introduce new IZkStateListener to helix. The new IZkStateListener adds new method
handleNewSession(String sessionId), and removes the old method handleNewSession().
- Add default implementations I0ItecIZkStateListenerHelixImpl in IZkStateListener
and IZkStateListenerI0ItecImpl in ZkClient for backward compatibility.
- Add session id to ZkEvent as a private field to help debug ZkEvent.
- Add unit tests to test subscribe/unsubscribe state changes.
@huizhilu huizhilu deleted the IZkStateListener branch February 11, 2020 08:08
mgao0 pushed a commit to mgao0/helix that referenced this pull request Mar 6, 2020
…Session for ZkHelixManager (apache#644)

I0Itec IZkStateListener doesn't have an API handleNewSession(sessionId) to handle session aware operation,
which is needed to fix session race condition for creating ephemeral node in ZkClient.
So this new IZkStateListener interface is introduced to provide session aware handleNewSession method for ZkHelixManager.

Changelist:
- Introduce new IZkStateListener to helix. The new IZkStateListener adds new method
handleNewSession(String sessionId), and removes the old method handleNewSession().
- Add default implementations I0ItecIZkStateListenerHelixImpl in IZkStateListener
and IZkStateListenerI0ItecImpl in ZkClient for backward compatibility.
- Add session id to ZkEvent as a private field to help debug ZkEvent.
- Add unit tests to test subscribe/unsubscribe state changes.
huizhilu added a commit to huizhilu/helix that referenced this pull request Aug 16, 2020
…Session for ZkHelixManager (apache#644)

I0Itec IZkStateListener doesn't have an API handleNewSession(sessionId) to handle session aware operation,
which is needed to fix session race condition for creating ephemeral node in ZkClient.
So this new IZkStateListener interface is introduced to provide session aware handleNewSession method for ZkHelixManager.

Changelist:
- Introduce new IZkStateListener to helix. The new IZkStateListener adds new method
handleNewSession(String sessionId), and removes the old method handleNewSession().
- Add default implementations I0ItecIZkStateListenerHelixImpl in IZkStateListener
and IZkStateListenerI0ItecImpl in ZkClient for backward compatibility.
- Add session id to ZkEvent as a private field to help debug ZkEvent.
- Add unit tests to test subscribe/unsubscribe state changes.
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.

Customize IZkStateListener

4 participants