[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r181179924
##
File path: assemble/bin/accumulo-cluster
##
@@ -244,7 +244,7 @@ function kill_all() {
done
echo "Cleaning all server entries in ZooKeeper"
- ${accumulo_cmd} org.apache.accumulo.server.util.ZooZap -master -tservers
-tracers --site-file "${conf}/accumulo-site.xml"
+ ${accumulo_cmd} org.apache.accumulo.server.util.ZooZap -master -tservers
-tracers
Review comment:
Okay. That code will grab the site file from the class path, which is good.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r180586201
##
File path:
server/base/src/main/java/org/apache/accumulo/server/cli/ClientOpts.java
##
@@ -34,6 +32,10 @@ public Instance getInstance() {
if (instance == null) {
return HdfsZooInstance.getInstance();
}
-return new ZooKeeperInstance(this.getClientConfiguration());
+try {
+ return getConnector().getInstance();
+} catch (Exception e) {
+ throw new RuntimeException(e);
Review comment:
Same here, with respect to bad exception handling.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r180586097
##
File path:
server/base/src/main/java/org/apache/accumulo/server/cli/ClientOnRequiredTable.java
##
@@ -36,6 +35,10 @@ synchronized public Instance getInstance() {
if (instance == null) {
return cachedInstance = HdfsZooInstance.getInstance();
}
-return cachedInstance = new ZooKeeperInstance(getClientConfiguration());
+try {
+ return cachedInstance = getConnector().getInstance();
+} catch (Exception e) {
+ throw new RuntimeException(e);
Review comment:
This is a bad pattern to follow, for a few reasons:
1. We have multi-catch blocks now, so there's no reason to do blanket catch
like this,
2. This will unnecessarily wrap `RuntimeException`s with another
`RuntimeException`, and
3. You should never call `new RuntimeException(...)`; instead, pick a
specific runtime exception appropriate to the circumstance.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r180582894
##
File path: assemble/bin/accumulo-cluster
##
@@ -244,7 +244,7 @@ function kill_all() {
done
echo "Cleaning all server entries in ZooKeeper"
- ${accumulo_cmd} org.apache.accumulo.server.util.ZooZap -master -tservers
-tracers --site-file "${conf}/accumulo-site.xml"
+ ${accumulo_cmd} org.apache.accumulo.server.util.ZooZap -master -tservers
-tracers
Review comment:
Does the `ZooZap` command no longer need access to the site file? I would
imagine it is one of those utilities that needs the `instance.secret`. I don't
think a client configuration file will be an adequate substitute.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r180584057
##
File path: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java
##
@@ -252,8 +182,7 @@ public void updateKerberosCredentials() {
* overwrite the options if the user has provided something specifically.
*/
public void updateKerberosCredentials(boolean clientSaslEnabled) {
-if ((saslEnabled || clientSaslEnabled) && null == tokenClassName) {
- tokenClassName = KerberosToken.CLASS_NAME;
+if (saslEnabled || clientSaslEnabled) {
Review comment:
Why remove the `null == tokenClassName` portion? If the code was changed so
that the tokenClassName is never null... then is this block needed at all
anymore?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r180584941
##
File path: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java
##
@@ -282,15 +211,24 @@ public void parseArgs(String programName, String[] args,
Object... others) {
updateKerberosCredentials(clientConfigFile);
}
+ private ConnectionInfo cachedInfo = null;
+ private Connector cachedConnector = null;
protected Instance cachedInstance = null;
- protected ClientConfiguration cachedClientConfig = null;
+ private Properties cachedProps = null;
synchronized public Instance getInstance() {
-if (cachedInstance != null)
- return cachedInstance;
-if (mock)
- return cachedInstance = DeprecationUtil.makeMockInstance(instance);
-return cachedInstance = new
ZooKeeperInstance(this.getClientConfiguration());
+if (cachedInstance == null) {
+ if (mock) {
+cachedInstance = DeprecationUtil.makeMockInstance(instance);
Review comment:
Mock is deprecated. If we're changing this code anyway, it's probably better
to go ahead and phase it out, if it isn't going to add too many complications.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r180584037
##
File path: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java
##
@@ -195,19 +129,15 @@ public AuthenticationToken getToken() {
@Parameter(names = {"-fake", "--mock"}, description = "Use a mock Instance")
public boolean mock = false;
- @Parameter(names = "--site-file",
- description = "Read the given accumulo site file to find the accumulo
instance")
- public String siteFile = null;
-
@Parameter(names = "--ssl", description = "Connect to accumulo over SSL")
public boolean sslEnabled = false;
@Parameter(names = "--sasl", description = "Connecto to Accumulo using SASL
(supports Kerberos)")
public boolean saslEnabled = false;
@Parameter(names = "--config-file", description = "Read the given client
config file. "
- + "If omitted, the path searched can be specified with
$ACCUMULO_CLIENT_CONF_PATH, which "
- + "defaults to
~/.accumulo/config:$ACCUMULO_CONF_DIR/client.conf:/etc/accumulo/client.conf")
+ + "If omitted, the following paths will be searched
~/.accumulo/accumulo-client.properties:"
+ +
"$ACCUMULO_CONF_DIR/accumulo-client.properties:/etc/accumulo/accumulo-client.properties")
Review comment:
I'm not a fan of the file names ending in `.properties`. It's longer to
type, and it exposes implementation detail a bit. I think I prefer the old
`client.conf` file name, or maybe `accumulo-client.conf`. Changing conventions
like this can be a pain for sysadmins... although client configuration was
probably never used much because it wasn't a very mature feature before, so
maybe it's okay to change the convention. Still, I'm not a fan of the
`.properties` extension.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ctubbsii commented on a change in pull request #419: #408 - Removed ClientConfiguration from ClientOpts
ctubbsii commented on a change in pull request #419: #408 - Removed
ClientConfiguration from ClientOpts
URL: https://github.com/apache/accumulo/pull/419#discussion_r180585375
##
File path: core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java
##
@@ -338,69 +276,66 @@ public void setSecurePassword(Password securePassword) {
this.securePassword = securePassword;
}
- public String getTokenClassName() {
-return tokenClassName;
+ public ConnectionInfo getConnectionInfo() {
+if (cachedInfo == null) {
+ cachedInfo =
Connector.builder().usingProperties(getClientProperties()).info();
+}
+return cachedInfo;
}
public Connector getConnector() throws AccumuloException,
AccumuloSecurityException {
-return getInstance().getConnector(getPrincipal(), getToken());
- }
-
- public ClientConfiguration getClientConfiguration() throws
IllegalArgumentException {
-if (cachedClientConfig != null)
- return cachedClientConfig;
-
-ClientConfiguration clientConfig;
-try {
- if (clientConfigFile == null)
-clientConfig = ClientConfiguration.loadDefault();
- else
-clientConfig = ClientConfiguration.fromFile(new
File(clientConfigFile));
-} catch (Exception e) {
- throw new IllegalArgumentException(e);
+if (cachedConnector == null) {
+ cachedConnector =
Connector.builder().usingConnectionInfo(getConnectionInfo()).build();
}
-if (sslEnabled)
- clientConfig.setProperty(ClientProperty.INSTANCE_RPC_SSL_ENABLED,
"true");
-
-if (saslEnabled)
- clientConfig.setProperty(ClientProperty.INSTANCE_RPC_SASL_ENABLED,
"true");
+return cachedConnector;
+ }
-if (siteFile != null) {
- AccumuloConfiguration config = new AccumuloConfiguration() {
-Configuration xml = new Configuration();
-{
- xml.addResource(new Path(siteFile));
+ public String getClientConfigFile() {
+if (clientConfigFile == null) {
+ List searchPaths = new LinkedList<>();
+ searchPaths.add(System.getProperty("user.home") +
"/.accumulo/accumulo-client.properties");
+ if (System.getenv("ACCUMULO_CONF_DIR") != null) {
Review comment:
This regresses in our attempt to phase out the use of environment variables
in our Java code.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
