You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/04/13 22:31:22 UTC
[GitHub] [accumulo] karthick-rn opened a new pull request #1586: Fix for
#1578
karthick-rn opened a new pull request #1586: Fix for #1578
URL: https://github.com/apache/accumulo/pull/1586
This PR addresses the issue described in #1578. The code changes has been successfully tested against TLS & non-TLS configured ZK setup. Please review & let me know for any questions/suggestions.
Like I mentioned in #1578 regarding the warning message during start/stop of services I'll create a separate issue to investigate.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] karthick-rn commented on a change in pull request #1586:
Fix Accumulo hanging when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
karthick-rn commented on a change in pull request #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#discussion_r408388864
##########
File path: core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
##########
@@ -65,7 +65,8 @@
* In this mode singletons are never disabled unless the mode is set back to CLIENT. The user
* can do this by using util.CleanUp (an old API created for users).
*/
- CONNECTOR
+ CONNECTOR,
+ CLOSED
Review comment:
Added comments to explain CLOSED mode
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1586:
Fix for #1578
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1586: Fix for #1578
URL: https://github.com/apache/accumulo/pull/1586#discussion_r407831441
##########
File path: core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
##########
@@ -65,7 +65,8 @@
* In this mode singletons are never disabled unless the mode is set back to CLIENT. The user
* can do this by using util.CleanUp (an old API created for users).
*/
- CONNECTOR
+ CONNECTOR,
+ CLOSED
Review comment:
As I understand it, the basis of this pull request is to create this CLOSED mode. What is this CLOSED mode for? This could use a javadoc similar to the other modes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] karthick-rn commented on issue #1586: Fix Accumulo
hanging when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
karthick-rn commented on issue #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#issuecomment-613668692
> Hi @karthick-rn ; thanks for the pull request. Here are some tips to improve your git log messages: https://chris.beams.io/posts/git-commit/
Thanks @ctubbsii for sharing the link. Going forward I'll ensure the git log messages are as described on the link.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] karthick-rn commented on a change in pull request #1586:
Fix Accumulo hanging when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
karthick-rn commented on a change in pull request #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#discussion_r408388864
##########
File path: core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
##########
@@ -65,7 +65,8 @@
* In this mode singletons are never disabled unless the mode is set back to CLIENT. The user
* can do this by using util.CleanUp (an old API created for users).
*/
- CONNECTOR
+ CONNECTOR,
+ CLOSED
Review comment:
Added comments to explain CLOSED mode, also updated the CLIENT mode comments.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1586: Fix Accumulo hanging
when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#issuecomment-613639163
> Another way to make this change cleaner would be to call `SingletonManager.setMode(CLOSED)` in `ServerContext.close()`. However doing this may destabilize existing code using ServerContext. To avoid the destabilization, could create an AdminUtilContext that extends ServerContext AND calls `SingletonManager.setMode(CLOSED)` in its close method. Then admin util like the ones changed in this PR could do something like the following.
We've talked about creating a Context for utilities before. I think it probably makes sense to do that for this reason as well.
Another way to simplify this is to let the context explicitly manage the ZooKeeper object, rather than use the SingletonManager stuff at all. In my view, the SingletonManager was a short-term hack to manage static object lifecycles, when the long-term fix is to avoid static objects. The context object is the primary lifecycle object, so we really should only be getting our ZooKeeper object from that. The context itself should close the ZooKeeper object when it is closed.
> Just sharing my thoughts, not suggesting we do any of this at this point.
Yeah, all these ideas can be left for future changes/improvements. The approach taken by this PR is fine with me for 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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1586:
Fix for #1578
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1586: Fix for #1578
URL: https://github.com/apache/accumulo/pull/1586#discussion_r407833135
##########
File path: core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
##########
@@ -148,6 +149,8 @@ public static long getReservationCount() {
* Change how singletons are managed. The default mode is {@link Mode#CLIENT}
*/
public static synchronized void setMode(Mode mode) {
+ if (SingletonManager.mode == Mode.CLOSED && mode != Mode.CLOSED)
+ throw new IllegalStateException("Cannot leave closed mode once entered");
Review comment:
Would it be okay to just return if the mode parameter is already the current mode?
```suggestion
if (SingletonManager.mode == mode)
return;
if (SingletonManager.mode == Mode.CLOSED)
throw new IllegalStateException("Cannot leave closed mode once entered");
```
Or is it necessary to fall through and do something if they are both `CLOSED` already?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on a change in pull request
#1586: Fix for #1578
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1586: Fix for #1578
URL: https://github.com/apache/accumulo/pull/1586#discussion_r408150853
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##########
@@ -275,6 +277,8 @@ public void execute(final String[] args) {
} catch (Exception e) {
log.error("{}", e.getMessage(), e);
System.exit(3);
+ } finally {
+ SingletonManager.setMode(Mode.CLOSED);
Review comment:
> Does this prevent this code from being called from a process that might have other Accumulo client Connectors?
Yes I think it would. I don't think this situation happens currently. If it ever did in the future, a clear error message would occur making it easy to track down. Since this not public API, its always easy to change this code later if the hypothetical situation does happen.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1586:
Fix Accumulo hanging when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#discussion_r408394817
##########
File path: core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
##########
@@ -170,8 +170,10 @@ public static synchronized void setMode(Mode mode) {
transitionedFromClientToConnector = true;
}
- // always allow transition to closed and only allow transition to client/connector when the
- // current mode is not server
+ /**
Review comment:
This isn't a javadoc comment.
```suggestion
/*
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii merged pull request #1586: Fix Accumulo hanging
when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1586: Fix Accumulo hanging
when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#issuecomment-613674197
Merged. Thanks for the PR @karthick-rn . I know you've contributed a few times, but I didn't see you listed on https://accumulo.apache.org/people/, and didn't see a previous invitation to add yourself there. So apologies if you've received this information before:
If you wish to be added as a contributor to https://accumulo.apache.org/people/ , please open a pull request to add yourself at https://github.com/apache/accumulo-website/edit/master/pages/people.md and leave a reference to `apache/accumulo#1586` in your commit log.
If you intend to be a regular contributor to Accumulo projects, please consider subscribing to our developer mailing list (https://accumulo.apache.org/contact-us/) and introducing yourself. :smiley_cat:
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1586:
Fix for #1578
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1586: Fix for #1578
URL: https://github.com/apache/accumulo/pull/1586#discussion_r407834068
##########
File path: core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
##########
@@ -161,7 +164,7 @@ public static synchronized void setMode(Mode mode) {
}
// do not change from server mode, its a terminal mode that can not be left once entered
- if (SingletonManager.mode != Mode.SERVER) {
+ if (SingletonManager.mode != Mode.SERVER || mode == Mode.CLOSED) {
Review comment:
The comment above this line covers the left hand side, but doesn't explain the right hand side of this `||` expression. Should it say something about `always allow changing state to CLOSED`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] keith-turner commented on issue #1586: Fix Accumulo
hanging when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
keith-turner commented on issue #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#issuecomment-613601627
> I feel like the three affected tools could simply have avoided using ZooReaderWriter and ZooSession and instead just used ZooKeeper directly.
This may lead to a lot of duplication and/or require a large refactoring of code to reuse things like the authentication code for ZK. Not sure.
Another way to make this change cleaner would be to call `SingletonManager.setMode(CLOSED)` in `ServerContext.close()`. However doing this may destabilize existing code using ServerContext. To avoid the destabilization, could create an AdminUtilContext that extends ServerContext AND calls `SingletonManager.setMode(CLOSED)` in its close method. Then admin util like the ones changed in this PR could do something like the following.
```java
try(AdminUtilContext ctx = new AdminUtilContext()) {
// do admin stuff
}
```
Just sharing my thoughts, not suggesting we do any of this at this point.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1586: Fix Accumulo hanging
when TLS enabled on ZooKeeper
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1586: Fix Accumulo hanging when TLS enabled on ZooKeeper
URL: https://github.com/apache/accumulo/pull/1586#issuecomment-613650094
I'm preparing to 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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1586:
Fix for #1578
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1586: Fix for #1578
URL: https://github.com/apache/accumulo/pull/1586#discussion_r407834757
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##########
@@ -275,6 +277,8 @@ public void execute(final String[] args) {
} catch (Exception e) {
log.error("{}", e.getMessage(), e);
System.exit(3);
+ } finally {
+ SingletonManager.setMode(Mode.CLOSED);
Review comment:
Does this prevent this code from being called from a process that might have other Accumulo client Connectors? If so, this is a bit concerning as a restriction.
(Same comment for ZooZap and SetGoalState)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1586: Fix for #1578
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1586: Fix for #1578
URL: https://github.com/apache/accumulo/pull/1586#issuecomment-613194113
Hi @karthick-rn ; thanks for the pull request. Here are some tips to improve your git log messages: https://chris.beams.io/posts/git-commit/
----------------------------------------------------------------
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
With regards,
Apache Git Services