You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/10/15 03:48:49 UTC
[GitHub] [zookeeper] hanm opened a new pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
hanm opened a new pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505
This is a follow up of ZOOKEEPER-3948. Here we enable ZooKeeperServerController to be able to expire a global or local session. This is very useful in our experience in integration testing when we want a controlled session expiration mechanism. This is done by having session tracker exposing both global and local session stats, so a zookeeper server can expire the sessions in the controller.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#issuecomment-722683038
@eolivelli @ztzg ptal
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#discussion_r505152085
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/controller/ZooKeeperServerController.java
##########
@@ -164,5 +164,23 @@ public void processCommand(ControlCommand command) {
}
}
+ private ZooKeeperServer getServer() {
+ return quorumPeer.getActiveServer();
+ }
+
+ private void expireSession(long sessionId) {
+ getServer().expire(sessionId);
Review comment:
`getServer(this)` will not be null which is guaranteed by `ZooKeeperServerController::isReady` (controller will not in action if it's not ready. So this is not a concern for our case (looks like muse dev analysis is not context / flow sensitive).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] muse-dev[bot] commented on a change in pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#discussion_r505149927
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/controller/ZooKeeperServerController.java
##########
@@ -164,5 +164,23 @@ public void processCommand(ControlCommand command) {
}
}
+ private ZooKeeperServer getServer() {
+ return quorumPeer.getActiveServer();
+ }
+
+ private void expireSession(long sessionId) {
+ getServer().expire(sessionId);
+ }
+
+ private void expireAllSessions() {
+ for (Long sessionId : getServer().getSessionTracker().localSessions()) {
Review comment:
*NULL_DEREFERENCE:* object returned by `getServer(this)` could be null and is dereferenced at line 176.
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/controller/ZooKeeperServerController.java
##########
@@ -164,5 +164,23 @@ public void processCommand(ControlCommand command) {
}
}
+ private ZooKeeperServer getServer() {
+ return quorumPeer.getActiveServer();
+ }
+
+ private void expireSession(long sessionId) {
+ getServer().expire(sessionId);
Review comment:
*NULL_DEREFERENCE:* object returned by `getServer(this)` could be null and is dereferenced at line 172.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg closed pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
ztzg closed pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#issuecomment-844891546
> This PR was reviewed and ready to merge. @ztzg or @maoling Can any of you please help merge this?
Looking into it now…
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#discussion_r523364242
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LocalSessionTracker.java
##########
@@ -26,9 +28,11 @@
* Local session tracker.
*/
public class LocalSessionTracker extends SessionTrackerImpl {
+ ConcurrentMap<Long, Integer> sessionsWithTimeouts;
Review comment:
It was made package private because there was no need for sub classes of session tracker interface to access this. I agree for this patch it's better to change the accessor to protected which leads to less code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#discussion_r523364277
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LocalSessionTracker.java
##########
@@ -45,4 +49,7 @@ public long createSession(int sessionTimeout) {
return sessionId;
}
+ public Set<Long> localSessions() {
+ return (sessionsWithTimeouts == null) ? Collections.emptySet() : sessionsWithTimeouts.keySet();
Review comment:
good catch, I removed the check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#issuecomment-844913268
> This PR was reviewed and ready to merge. @ztzg or @maoling Can any of you please help merge this?
Okay, @hanm; this is now in `master` and `branch-3.7`. (I included it in the latter as ZOOKEEPER-3948 also is in that branch.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#issuecomment-844301520
This PR was reviewed and ready to merge. @ztzg or @maoling Can any of you please help merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#discussion_r521289587
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LocalSessionTracker.java
##########
@@ -26,9 +28,11 @@
* Local session tracker.
*/
public class LocalSessionTracker extends SessionTrackerImpl {
+ ConcurrentMap<Long, Integer> sessionsWithTimeouts;
Review comment:
Why not making the superclass' field `protected final` instead of `private final`? Is this being made package-private on purpose?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LocalSessionTracker.java
##########
@@ -45,4 +49,7 @@ public long createSession(int sessionTimeout) {
return sessionId;
}
+ public Set<Long> localSessions() {
+ return (sessionsWithTimeouts == null) ? Collections.emptySet() : sessionsWithTimeouts.keySet();
Review comment:
Unless I am missing something, and just FYI, `sessionsWithTimeouts` cannot be `null` here as it would have caused the `super` constructor to throw.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
hanm commented on pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#issuecomment-845659861
thanks @ztzg
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] hanm commented on a change in pull request #1505: ZOOKEEPER-3970: Enable ZooKeeperServerController to expire session.
Posted by GitBox <gi...@apache.org>.
hanm commented on a change in pull request #1505:
URL: https://github.com/apache/zookeeper/pull/1505#discussion_r518408595
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/controller/ZooKeeperServerController.java
##########
@@ -164,5 +164,23 @@ public void processCommand(ControlCommand command) {
}
}
+ private ZooKeeperServer getServer() {
+ return quorumPeer.getActiveServer();
+ }
+
+ private void expireSession(long sessionId) {
+ getServer().expire(sessionId);
Review comment:
just to mute muse-dev, i added a state check here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org