[GitHub] jmark99 commented on a change in pull request #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
jmark99 commented on a change in pull request #370: ACCUMULO-4772 Update shell
to use NewTableConfiguration methods
URL: https://github.com/apache/accumulo/pull/370#discussion_r165247235
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +169,83 @@ public int execute(final String fullCommand, final
CommandLine cl, final Shell s
return 0;
}
+ /**
+ * Add supplied locality groups information to a NewTableConfiguration
object.
+ *
+ * Used in conjunction with createtable shell command to allow locality
groups to be configured upon table creation.
+ */
+ private NewTableConfiguration setLocalityForNewTable(CommandLine cl,
NewTableConfiguration ntc) {
+HashMap localityGroupMap = new HashMap<>();
+String[] options =
cl.getOptionValues(createTableOptLocalityProps.getOpt());
+for (String localityInfo : options) {
+ final String parts[] = localityInfo.split("=", 2);
+ if (parts.length < 2)
+throw new IllegalArgumentException("Missing '=' or there are spaces
between entries");
+ final String groupName = parts[0];
+ final HashSet colFams = new HashSet<>();
+ for (String family : parts[1].split(","))
+colFams.add(new Text(family.getBytes(Shell.CHARSET)));
+ if (localityGroupMap.containsKey(groupName))
+throw new IllegalArgumentException("Duplicate locality group name
found. Group names must be unique");
+ localityGroupMap.put(groupName, colFams);
+}
+ntc.setLocalityGroups(localityGroupMap);
+return ntc;
+ }
+
+ /**
+ * Add supplied iterator information to NewTableConfiguration object.
+ *
+ * Used in conjunction with createtable shell command to allow an iterator
to be configured upon table creation.
+ */
+ private NewTableConfiguration attachIteratorToNewTable(CommandLine cl, Shell
shellState, NewTableConfiguration ntc) {
+if (shellState.iteratorProfiles.size() == 0)
+ throw new IllegalArgumentException("No shell iterator profiles have been
created.");
+String[] options =
cl.getOptionValues(createTableOptIteratorProps.getOpt());
+for (String profileInfo : options) {
+ String[] parts = profileInfo.split(":", 2);
+ if (parts.length < 2)
+throw new IllegalArgumentException("Missing scope or there are spaces
between parameters");
+ // get profile name
+ String profileName = parts[0];
+ IteratorSetting iteratorSetting =
shellState.iteratorProfiles.get(profileName).get(0);
+ if (iteratorSetting == null)
+throw new IllegalArgumentException("Provided iterator profile, " +
profileName + ", does not exist");
+ // parse scope info
+ List scopeList = Arrays.asList(parts[1].split(","));
Review comment:
@keith-turner, I refactored the attachIteratorToNewTable method . Take a
look and let me know your thoughts. I made use of
IteratorUtil.IteratorScope.valueOf() for the scopeList check and rearranged the
order of several checks to handle more error conditions. I didn't make use
lowercase as all arguments for other shell methods use lowercase and It seemed
more consistent to require users to provide lowercase parameters. If you feel
strongly about handling mixed case args you can let me know and I can update.
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] mikewalch commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
mikewalch commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165197001
##
File path:
core/src/main/java/org/apache/accumulo/core/client/ConnectionInfo.java
##
@@ -0,0 +1,21 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.core.client;
+
+public interface ConnectionInfo {
+
+}
Review comment:
`ConnectionInfo` lacks methods as I am not sure what to expose to users. I
don't want to expose most of the methods in `ConnectionInfoImpl`. However, I
would say `ConnectionInfo` is not complete as it should have some methods for
users to get basic information on their connection and all client properties.
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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector URL: https://github.com/apache/accumulo/pull/361#discussion_r165191818 ## File path: core/src/main/java/org/apache/accumulo/core/client/mapreduce/AccumuloOutputFormat.java ## @@ -37,6 +38,7 @@ import org.apache.accumulo.core.client.admin.DelegationTokenConfig; import org.apache.accumulo.core.client.admin.SecurityOperations; import org.apache.accumulo.core.client.impl.AuthenticationTokenIdentifier; +import org.apache.accumulo.core.client.impl.ConnectionInfoImpl; Review comment: Using the interface as the type should eliminate the need to depend on the impl as well. 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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165190520
##
File path:
core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java
##
@@ -193,4 +221,151 @@ public synchronized ReplicationOperations
replicationOperations() {
return replicationops;
}
+
+ public static class ConnectorBuilderImpl implements InstanceArgs,
PropertyOptions, ConnectionInfoOptions, AuthenticationArgs, ConnectionOptions,
SslOptions,
+ SaslOptions, ConnectorFactory {
+
+private ConnectionInfoImpl info = new ConnectionInfoImpl();
Review comment:
Shouldn't this type be ConnectionInfo?
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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165191859
##
File path:
core/src/main/java/org/apache/accumulo/core/client/mapreduce/AccumuloOutputFormat.java
##
@@ -81,6 +81,21 @@
private static final Class CLASS = AccumuloOutputFormat.class;
protected static final Logger log = Logger.getLogger(CLASS);
+ /**
+ * Set the connection information needed to communicate with Accumulo in
this job.
+ *
+ * @param job
+ * Hadoop job to be configured
+ * @param connectionInfo
+ * Accumulo connection information
+ * @since 2.0.0
+ */
+ public static void setConnectionInfo(Job job, ConnectionInfo connectionInfo)
throws AccumuloSecurityException {
+ConnectionInfoImpl info = (ConnectionInfoImpl) connectionInfo;
Review comment:
And here.
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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165191356
##
File path:
core/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java
##
@@ -115,6 +117,21 @@ public static String getClassLoaderContext(JobConf job) {
return InputConfigurator.getClassLoaderContext(CLASS, job);
}
+ /**
+ * Sets connection information needed to communicate with Accumulo for this
job
+ *
+ * @param job
+ * Hadoop job instance to be configured
+ * @param connectionInfo
+ * Connection information for Accumulo
+ * @since 2.0.0
+ */
+ public static void setConnectionInfo(JobConf job, ConnectionInfo
connectionInfo) throws AccumuloSecurityException {
+ConnectionInfoImpl info = (ConnectionInfoImpl) connectionInfo;
Review comment:
Another spot where the type should be the interface not the impl.
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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165189598
##
File path:
core/src/main/java/org/apache/accumulo/core/client/ConnectionInfo.java
##
@@ -0,0 +1,21 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.core.client;
+
+public interface ConnectionInfo {
+
+}
Review comment:
Is this unfinished? If not, should have all the required methods here.
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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165192489
##
File path:
test/src/main/java/org/apache/accumulo/test/functional/ConnectorIT.java
##
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.test.functional;
+
+import java.util.Properties;
+
+import org.apache.accumulo.core.client.ConnectionInfo;
+import org.apache.accumulo.core.client.Connector;
+import org.apache.accumulo.core.client.impl.ConnectionInfoImpl;
+import org.apache.accumulo.core.client.security.tokens.PasswordToken;
+import org.apache.accumulo.core.conf.ClientProperty;
+import org.apache.accumulo.harness.AccumuloClusterHarness;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ConnectorIT extends AccumuloClusterHarness {
+
+ @Test
+ public void testConnectorBuilder() throws Exception {
+Connector c = getConnector();
+String instanceName = c.getInstance().getInstanceName();
+String zookeepers = c.getInstance().getZooKeepers();
+final String user = "testuser";
+final String password = "testpassword";
+c.securityOperations().createLocalUser(user, new PasswordToken(password));
+
+Connector conn = Connector.builder().forInstance(instanceName,
zookeepers).usingBasicCredentials(user, password).build();
+
+Assert.assertEquals(instanceName, conn.getInstance().getInstanceName());
+Assert.assertEquals(zookeepers, conn.getInstance().getZooKeepers());
+Assert.assertEquals(user, conn.whoami());
+
+ConnectionInfo info = Connector.builder().forInstance(instanceName,
zookeepers).usingBasicCredentials(user, password).info();
+ConnectionInfoImpl impl = (ConnectionInfoImpl) info;
Review comment:
Same with my comments above, should only see the interface at this level.
But the builder method above looks awesome!!
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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165191146
##
File path:
core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java
##
@@ -193,4 +221,151 @@ public synchronized ReplicationOperations
replicationOperations() {
return replicationops;
}
+
+ public static class ConnectorBuilderImpl implements InstanceArgs,
PropertyOptions, ConnectionInfoOptions, AuthenticationArgs, ConnectionOptions,
SslOptions,
+ SaslOptions, ConnectorFactory {
+
+private ConnectionInfoImpl info = new ConnectionInfoImpl();
+
+@Override
+public Connector build() throws AccumuloException,
AccumuloSecurityException {
+ return info.getConnector();
+}
+
+@Override
+public ConnectionInfo info() {
+ return info;
+}
+
+@Override
+public AuthenticationArgs forInstance(String instanceName, String
zookeepers) {
+ info.setProperty(ClientProperty.INSTANCE_NAME, instanceName);
+ info.setProperty(ClientProperty.INSTANCE_ZOOKEEPERS, zookeepers);
+ return this;
+}
+
+@Override
+public SslOptions withTruststore(String path) {
+ info.setProperty(ClientProperty.SSL_TRUSTSTORE_PATH, path);
+ return this;
+}
+
+@Override
+public SslOptions withTruststore(String path, String password, String
type) {
+ info.setProperty(ClientProperty.SSL_TRUSTSTORE_PATH, path);
+ info.setProperty(ClientProperty.SSL_TRUSTSTORE_PASSWORD, password);
+ info.setProperty(ClientProperty.SSL_TRUSTSTORE_TYPE, type);
+ return this;
+}
+
+@Override
+public SslOptions withKeystore(String path) {
+ info.setProperty(ClientProperty.SSL_KEYSTORE_PATH, path);
+ return this;
+}
+
+@Override
+public SslOptions withKeystore(String path, String password, String type) {
+ info.setProperty(ClientProperty.SSL_KEYSTORE_PATH, path);
+ info.setProperty(ClientProperty.SSL_KEYSTORE_PASSWORD, password);
+ info.setProperty(ClientProperty.SSL_KEYSTORE_TYPE, type);
+ return this;
+}
+
+@Override
+public SslOptions useJsse() {
+ info.setProperty(ClientProperty.SSL_USE_JSSE, "true");
+ return this;
+}
+
+@Override
+public ConnectionOptions withZkTimeout(int timeout) {
+ info.setProperty(ClientProperty.INSTANCE_ZOOKEEPERS_TIMEOUT_SEC,
Integer.toString(timeout));
+ return this;
+}
+
+@Override
+public SslOptions withSsl() {
+ info.setProperty(ClientProperty.SSL_ENABLED, "true");
+ return this;
+}
+
+@Override
+public SaslOptions withSasl() {
+ info.setProperty(ClientProperty.SASL_ENABLED, "true");
+ return this;
+}
+
+@Override
+public ConnectionOptions withBatchWriterConfig(BatchWriterConfig
batchWriterConfig) {
+ info.setProperty(ClientProperty.BATCH_WRITER_MAX_MEMORY_BYTES,
batchWriterConfig.getMaxMemory());
+ info.setProperty(ClientProperty.BATCH_WRITER_MAX_LATENCY_SEC,
batchWriterConfig.getMaxLatency(TimeUnit.SECONDS));
+ info.setProperty(ClientProperty.BATCH_WRITER_MAX_TIMEOUT_SEC,
batchWriterConfig.getTimeout(TimeUnit.SECONDS));
+ info.setProperty(ClientProperty.BATCH_WRITER_MAX_WRITE_THREADS,
batchWriterConfig.getMaxWriteThreads());
+ info.setProperty(ClientProperty.BATCH_WRITER_DURABILITY,
batchWriterConfig.getDurability().toString());
+ return this;
+}
+
+@Override
+public SaslOptions withPrimary(String kerberosServerPrimary) {
+ info.setProperty(ClientProperty.SASL_KERBEROS_SERVER_PRIMARY,
kerberosServerPrimary);
+ return this;
+}
+
+@Override
+public SaslOptions withQop(String qualityOfProtection) {
+ info.setProperty(ClientProperty.SASL_QOP, qualityOfProtection);
+ return this;
+}
+
+@Override
+public ConnectorFactory usingProperties(String configFile) {
+ Properties properties = new Properties();
+ try (InputStream is = new FileInputStream(configFile)) {
+properties.load(is);
+ } catch (IOException e) {
+throw new IllegalArgumentException(e);
+ }
+ return usingProperties(properties);
+}
+
+@Override
+public ConnectorFactory usingProperties(Properties properties) {
+ info = new ConnectionInfoImpl(properties);
+ return this;
+}
+
+@Override
+public ConnectionOptions usingBasicCredentials(String username,
CharSequence password) {
+ info.setProperty(ClientProperty.AUTH_TYPE, "basic");
+ info.setProperty(ClientProperty.AUTH_BASIC_USERNAME, username);
+ info.setProperty(ClientProperty.AUTH_BASIC_PASSWORD,
password.toString());
+ return this;
+}
+
+@Override
+public ConnectionOptions usingKerberosCredentials(String principal, String
keyTabFile) {
+ info.setProperty(ClientProperty.AUTH_TYPE, "kerberos");
+
[GitHub] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165191555
##
File path:
core/src/main/java/org/apache/accumulo/core/client/mapreduce/AbstractInputFormat.java
##
@@ -118,6 +120,21 @@ public static String getClassLoaderContext(JobContext
job) {
return InputConfigurator.getClassLoaderContext(CLASS,
job.getConfiguration());
}
+ /**
+ * Sets connection information needed to communicate with Accumulo for this
job
+ *
+ * @param job
+ * Hadoop job instance to be configured
+ * @param connectionInfo
+ * Connection information for Accumulo
+ * @since 2.0.0
+ */
+ public static void setConnectionInfo(Job job, ConnectionInfo connectionInfo)
throws AccumuloSecurityException {
+ConnectionInfoImpl info = (ConnectionInfoImpl) connectionInfo;
Review comment:
Here too.
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] milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create builder for Connector
milleruntime commented on a change in pull request #361: ACCUMULO-4784 - Create
builder for Connector
URL: https://github.com/apache/accumulo/pull/361#discussion_r165191471
##
File path:
core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloOutputFormat.java
##
@@ -80,6 +80,21 @@
private static final Class CLASS = AccumuloOutputFormat.class;
protected static final Logger log = Logger.getLogger(CLASS);
+ /**
+ * Set the connection information needed to communicate with Accumulo in
this job.
+ *
+ * @param job
+ * Hadoop job to be configured
+ * @param connectionInfo
+ * Accumulo connection information
+ * @since 2.0.0
+ */
+ public static void setConnectionInfo(JobConf job, ConnectionInfo
connectionInfo) throws AccumuloSecurityException {
+ConnectionInfoImpl info = (ConnectionInfoImpl) connectionInfo;
Review comment:
Here too.
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
Accumulo-Pull-Requests - Build # 1008 - Fixed
The Apache Jenkins build system has built Accumulo-Pull-Requests (build #1008) Status: Fixed Check console output at https://builds.apache.org/job/Accumulo-Pull-Requests/1008/ to view the results.
[GitHub] keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log messages
keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log
messages
URL: https://github.com/apache/accumulo/pull/368#discussion_r165152800
##
File path:
fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java
##
@@ -21,9 +21,11 @@
*/
public class RetryFactory {
public static final long DEFAULT_MAX_RETRIES = 10l, DEFAULT_START_WAIT =
250l, DEFAULT_WAIT_INCREMENT = 250l, DEFAULT_MAX_WAIT = 5000l;
- public static final RetryFactory DEFAULT_INSTANCE = new
RetryFactory(DEFAULT_MAX_RETRIES, DEFAULT_START_WAIT, DEFAULT_WAIT_INCREMENT,
DEFAULT_MAX_WAIT);
+ public static final long DEFAULT_LOG_INTERVAL = 10 * 60 * 1000;
Review comment:
1 to 3 minutes
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] ivakegg commented on a change in pull request #368: ACCUMULO-4777: fix log messages
ivakegg commented on a change in pull request #368: ACCUMULO-4777: fix log
messages
URL: https://github.com/apache/accumulo/pull/368#discussion_r165142919
##
File path:
fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java
##
@@ -21,9 +21,11 @@
*/
public class RetryFactory {
public static final long DEFAULT_MAX_RETRIES = 10l, DEFAULT_START_WAIT =
250l, DEFAULT_WAIT_INCREMENT = 250l, DEFAULT_MAX_WAIT = 5000l;
- public static final RetryFactory DEFAULT_INSTANCE = new
RetryFactory(DEFAULT_MAX_RETRIES, DEFAULT_START_WAIT, DEFAULT_WAIT_INCREMENT,
DEFAULT_MAX_WAIT);
+ public static final long DEFAULT_LOG_INTERVAL = 10 * 60 * 1000;
Review comment:
What were you thinking? 1m, 5m, 42s ?
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] mikewalch commented on issue #361: ACCUMULO-4784 - Create builder for Connector
mikewalch commented on issue #361: ACCUMULO-4784 - Create builder for Connector URL: https://github.com/apache/accumulo/pull/361#issuecomment-362019578 I pushed another commit. I think this PR is no longer a WIP and is ready for review. There is more that I want to do for the Connector builder but that can be done in later issues. 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] mikewalch commented on issue #361: ACCUMULO-4784 - Create builder for Connector
mikewalch commented on issue #361: ACCUMULO-4784 - Create builder for Connector URL: https://github.com/apache/accumulo/pull/361#issuecomment-362019578 I pushed another commit. I think this PR is no longer a WIP and is ready for review. There is more that I want to on the Connector builder but that can be follow on issues. 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] ivakegg commented on a change in pull request #368: ACCUMULO-4777: fix log messages
ivakegg commented on a change in pull request #368: ACCUMULO-4777: fix log
messages
URL: https://github.com/apache/accumulo/pull/368#discussion_r165137470
##
File path: fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java
##
@@ -125,12 +149,51 @@ public long retriesCompleted() {
}
public void waitForNextAttempt() throws InterruptedException {
-log.debug("Sleeping for " + currentWait + "ms before retrying operation");
+if (log.isDebugEnabled()) {
+ log.debug("Sleeping for " + currentWait + "ms before retrying operation:
" + getCaller());
+}
sleep(currentWait);
currentWait = Math.min(maxWait, currentWait + waitIncrement);
}
protected void sleep(long wait) throws InterruptedException {
Thread.sleep(wait);
}
+
+ public void logRetry(String message, Throwable t) {
+// log the first time, and then after every logInterval
+if (lastRetryLog < 0 || (System.currentTimeMillis() - lastRetryLog) >
logInterval) {
+ log.warn(getMessage(message), t);
+ lastRetryLog = System.currentTimeMillis();
+}
+ }
+
+ public void logRetry(String message) {
Review comment:
I like that
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
Accumulo-Pull-Requests - Build # 1007 - Failure
The Apache Jenkins build system has built Accumulo-Pull-Requests (build #1007) Status: Failure Check console output at https://builds.apache.org/job/Accumulo-Pull-Requests/1007/ to view the results.
Accumulo-Master - Build # 2246 - Unstable
The Apache Jenkins build system has built Accumulo-Master (build #2246) Status: Unstable Check console output at https://builds.apache.org/job/Accumulo-Master/2246/ to view the results.
[jira] [Resolved] (ACCUMULO-4778) Resolving table name to table id is expensive
[ https://issues.apache.org/jira/browse/ACCUMULO-4778?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Michael Miller resolved ACCUMULO-4778. -- Resolution: Fixed > Resolving table name to table id is expensive > - > > Key: ACCUMULO-4778 > URL: https://issues.apache.org/jira/browse/ACCUMULO-4778 > Project: Accumulo > Issue Type: Bug >Affects Versions: 1.7.3, 1.8.1 >Reporter: Keith Turner >Assignee: Michael Miller >Priority: Major > Labels: pull-request-available > Fix For: 1.7.4, 1.9.0, 2.0.0 > > Time Spent: 4h 20m > Remaining Estimate: 0h > > I was running a Fluo test application and profiling the tablet server and > Fluo worker. The Fluo worker does lots small scans against Accumulo. > Resolving table names to ids (which is done for each scan) was expensive > enough to make a significant showing in the profiling data. > I looked that the 1.8 code and it does the following to resolve a table name : > * reads over all cached table ids in zookeeper putting them in a treemap > * does a lookup in the treemap > Ideally the client code would keep a cache of name to id mappings and > invalidate them when something changes in zookeeper. The data in zookeeper > is stored by id, so it does need to be inverted to lookup by name. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log messages
keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log
messages
URL: https://github.com/apache/accumulo/pull/368#discussion_r165092154
##
File path:
fate/src/main/java/org/apache/accumulo/fate/zookeeper/RetryFactory.java
##
@@ -21,9 +21,11 @@
*/
public class RetryFactory {
public static final long DEFAULT_MAX_RETRIES = 10l, DEFAULT_START_WAIT =
250l, DEFAULT_WAIT_INCREMENT = 250l, DEFAULT_MAX_WAIT = 5000l;
- public static final RetryFactory DEFAULT_INSTANCE = new
RetryFactory(DEFAULT_MAX_RETRIES, DEFAULT_START_WAIT, DEFAULT_WAIT_INCREMENT,
DEFAULT_MAX_WAIT);
+ public static final long DEFAULT_LOG_INTERVAL = 10 * 60 * 1000;
Review comment:
This default seems a bit long to me.
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] keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log messages
keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log
messages
URL: https://github.com/apache/accumulo/pull/368#discussion_r165091158
##
File path: fate/src/test/java/org/apache/accumulo/fate/zookeeper/RetryTest.java
##
@@ -147,4 +154,344 @@ public void testUnlimitedRetry() {
unlimitedRetry2.useRetry();
}
}
+
+ @Test
+ public void testLogging() {
+TestLogger testLogger = new TestLogger();
+Retry.setLogger(testLogger);
+try {
+
+ // we want to do this for 5 second and observe the log messages
+ long start = System.currentTimeMillis();
+ long end = System.currentTimeMillis();
+ int i = 0;
+ for (; (end - start < 5000l) && (i < Integer.MAX_VALUE); i++) {
+unlimitedRetry1.logRetry("failure message");
+unlimitedRetry1.useRetry();
+end = System.currentTimeMillis();
+ }
+
+ // now observe what log messages we got which should be around 5 +- 1
+ Assert.assertTrue(i > 10);
+ Assert.assertTrue(testLogger.getMessages().size() >= 4 &&
testLogger.getMessages().size() <= 6);
+} finally {
+ Retry.setLogger(LoggerFactory.getLogger(Retry.class));
+}
+
+ }
+
+ private static class TestLogger implements Logger {
Review comment:
You may be able to check expected parameters with easymock.
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] keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log messages
keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log
messages
URL: https://github.com/apache/accumulo/pull/368#discussion_r165090810
##
File path: fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java
##
@@ -125,12 +149,51 @@ public long retriesCompleted() {
}
public void waitForNextAttempt() throws InterruptedException {
-log.debug("Sleeping for " + currentWait + "ms before retrying operation");
+if (log.isDebugEnabled()) {
+ log.debug("Sleeping for " + currentWait + "ms before retrying operation:
" + getCaller());
+}
sleep(currentWait);
currentWait = Math.min(maxWait, currentWait + waitIncrement);
}
protected void sleep(long wait) throws InterruptedException {
Thread.sleep(wait);
}
+
+ public void logRetry(String message, Throwable t) {
+// log the first time, and then after every logInterval
+if (lastRetryLog < 0 || (System.currentTimeMillis() - lastRetryLog) >
logInterval) {
+ log.warn(getMessage(message), t);
+ lastRetryLog = System.currentTimeMillis();
+}
+ }
+
+ public void logRetry(String message) {
Review comment:
Another possibility is to have the caller pass in a logger like
`logRetry(Logger log, String message)`. If this is done, would the
`getCaller()` method be needed? Also the `setLogger()` method would not be
needed for testing.
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] keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log messages
keith-turner commented on a change in pull request #368: ACCUMULO-4777: fix log
messages
URL: https://github.com/apache/accumulo/pull/368#discussion_r165087404
##
File path: fate/src/main/java/org/apache/accumulo/fate/zookeeper/Retry.java
##
@@ -100,10 +110,24 @@ void setMaxWait(long maxWait) {
this.maxWait = maxWait;
}
+ // Visible for testing
Review comment:
Guava has a visible for testing annotation that could be used here.
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
