You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2026/03/30 17:47:59 UTC
[PR] fixes ZooPropEditorIT [accumulo]
keith-turner opened a new pull request, #6275:
URL: https://github.com/apache/accumulo/pull/6275
This test had three prolems :
* It was using a remove -p command line option.
* It was reusing ZooPropEditor objects that had previously parsed command line options and this was causing problem.
* There was some non test code that was closing a reusable server context and this was causing odd test failures.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes ZooPropEditorIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6275:
URL: https://github.com/apache/accumulo/pull/6275#discussion_r3011834103
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServerKeywordExecutable.java:
##########
@@ -52,12 +52,10 @@ public synchronized ServerContext getServerContext() {
@Override
public void doExecute(JCommander cl, OPTS options) throws Exception {
// Login as the server on secure HDFS
- try (ServerContext context = getServerContext()) {
- AccumuloConfiguration conf = context.getConfiguration();
- if (conf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
- SecurityUtil.serverLogin(conf);
- }
- execute(cl, options);
+ AccumuloConfiguration conf = getServerContext().getConfiguration();
Review Comment:
The Accumulo server processes will get their ServerContext from AbstractServer, so this should only affect command line tools. For the classes that are using SKE.getServerContext(), this implies that there is a thread that still has a reference to the ServerContext *after* execute returns? Is this indicative of other things not being closed properly, like a ZooSession or Scanner?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes ZooPropEditorIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6275:
URL: https://github.com/apache/accumulo/pull/6275#discussion_r3011624338
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServerKeywordExecutable.java:
##########
@@ -52,12 +52,10 @@ public synchronized ServerContext getServerContext() {
@Override
public void doExecute(JCommander cl, OPTS options) throws Exception {
// Login as the server on secure HDFS
- try (ServerContext context = getServerContext()) {
- AccumuloConfiguration conf = context.getConfiguration();
- if (conf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
- SecurityUtil.serverLogin(conf);
- }
- execute(cl, options);
+ AccumuloConfiguration conf = getServerContext().getConfiguration();
Review Comment:
> There was some non test code that was closing a reusable server context and this was causing odd test failures.
Is that the basis for this change? This change will cause the ServerContext to not be closed before any CLI tool exits.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes ZooPropEditorIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6275:
URL: https://github.com/apache/accumulo/pull/6275#discussion_r3011777368
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServerKeywordExecutable.java:
##########
@@ -52,12 +52,10 @@ public synchronized ServerContext getServerContext() {
@Override
public void doExecute(JCommander cl, OPTS options) throws Exception {
// Login as the server on secure HDFS
- try (ServerContext context = getServerContext()) {
- AccumuloConfiguration conf = context.getConfiguration();
- if (conf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
- SecurityUtil.serverLogin(conf);
- }
- execute(cl, options);
+ AccumuloConfiguration conf = getServerContext().getConfiguration();
Review Comment:
Yeah, w/o this change some of the test will fail and I could not figure out exactly why but it was related to the server context being closed which this code was doing.
Looking through the code a lot of stuff calls SKE.getServerContext(). I considered making SKE.getServerContext() always return a new ServerContext object, but given how much its used that was probably not a good idea.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes ZooPropEditorIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #6275:
URL: https://github.com/apache/accumulo/pull/6275
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fixes ZooPropEditorIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6275:
URL: https://github.com/apache/accumulo/pull/6275#discussion_r3012183964
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServerKeywordExecutable.java:
##########
@@ -52,12 +52,10 @@ public synchronized ServerContext getServerContext() {
@Override
public void doExecute(JCommander cl, OPTS options) throws Exception {
// Login as the server on secure HDFS
- try (ServerContext context = getServerContext()) {
- AccumuloConfiguration conf = context.getConfiguration();
- if (conf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
- SecurityUtil.serverLogin(conf);
- }
- execute(cl, options);
+ AccumuloConfiguration conf = getServerContext().getConfiguration();
Review Comment:
Looked at it again and found the problem. The test wall reusing an object in a loop, I missed that before. So the first iteration of the loop closed the server context and that caused problems for subsequent iterations. I restored the try w/ resources to close the server context and reworked the test in e04808e
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org