[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_r164924347
##
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:
interesting thought
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_r164924273
##
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:
trueI was initially thinking about actually examining the contents of
the message which is why I went this routeI can rework it however
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_r164924121
##
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java
##
@@ -338,9 +338,10 @@ private void write(final Collection
sessions, boolean mincFinish,
success = (currentLogSet == logSetId.get());
}
} catch (DfsLogger.LogClosedException ex) {
-log.debug("Logs closed while writing, retrying attempt " +
writeRetry.retriesCompleted());
+writeRetry.logRetry("Logs closed while writing");
Review comment:
yea, I thought about that but also thought that perhaps that is going to far.
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_r164920845
##
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:
Random thought, not advocating for this. In 2.0.0-SNAP which uses java 8,
could pass in a lambda that does the actual logging. This code just decides
whether or not to call it. Not sure, but this may remove the need to inspect
the stack trace to get info about caller.
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_r164920148
##
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/log/TabletServerLogger.java
##
@@ -338,9 +338,10 @@ private void write(final Collection
sessions, boolean mincFinish,
success = (currentLogSet == logSetId.get());
}
} catch (DfsLogger.LogClosedException ex) {
-log.debug("Logs closed while writing, retrying attempt " +
writeRetry.retriesCompleted());
+writeRetry.logRetry("Logs closed while writing");
Review comment:
When I saw this code is made me think about also logging when the message is
different.
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_r164919814
##
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:
Could possibly use EasyMock to create a mock instance that expects warn
and/or debug to be called a certain number of times.
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-Master - Build # 2245 - Failure
The Apache Jenkins build system has built Accumulo-Master (build #2245) Status: Failure Check console output at https://builds.apache.org/job/Accumulo-Master/2245/ to view the results.
[GitHub] keith-turner commented on a change in pull request #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164916437
##
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:
I would validate by calling `IteratorUtil.IteratorScope.valueOf()` for each
item in scopeList. Could lower case the string before passing it to valueOf.
I think valueOf will throw an exception if its not a valid enum. Can also add
what valueOf returns to the set.
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164917471
##
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))
Review comment:
Instead of calling containsKey, could check that put returns null. However
the current code is probably more readable and it does not need to be
efficient.
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] 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_r164913889
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +168,67 @@ 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)));
+ 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(","));
+ if (scopeList.size() > 3) // max of three scope settings allowed
+throw new IllegalArgumentException("Too many scopes supplied");
+ EnumSet scopes =
EnumSet.noneOf(IteratorUtil.IteratorScope.class);
+ if (scopeList.contains("all") || scopeList.contains("scan"))
Review comment:
Added check to verify only valid scope arguments are accepted.
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] 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_r164913826
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +168,67 @@ 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)));
+ localityGroupMap.put(groupName, colFams);
Review comment:
Added check to verify group names cannot be duplicated.
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
[jira] [Updated] (ACCUMULO-4792) Improve crypto configuration checks
[ https://issues.apache.org/jira/browse/ACCUMULO-4792?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] ASF GitHub Bot updated ACCUMULO-4792: - Labels: pull-request-available (was: ) > Improve crypto configuration checks > --- > > Key: ACCUMULO-4792 > URL: https://issues.apache.org/jira/browse/ACCUMULO-4792 > Project: Accumulo > Issue Type: Improvement >Reporter: Nick Felts >Assignee: Nick Felts >Priority: Minor > Labels: pull-request-available > > The crypto module class and secret key encryption strategy class should each > be checked to ensure that if one is set not-null, the other is also enabled. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] PircDef opened a new pull request #371: ACCUMULO-4792 Improve crypto configuration checks
PircDef opened a new pull request #371: ACCUMULO-4792 Improve crypto configuration checks URL: https://github.com/apache/accumulo/pull/371 Added a check to the configuration sanity checker to ensure that, if a crypto module other than NullCryptoModule was selected, a SecretKeyEncryptionStrategy other than NullSecretKeyEncryptionStrategy must also be selected (and vice versa). 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
[jira] [Created] (ACCUMULO-4792) Improve crypto configuration checks
Nick Felts created ACCUMULO-4792: Summary: Improve crypto configuration checks Key: ACCUMULO-4792 URL: https://issues.apache.org/jira/browse/ACCUMULO-4792 Project: Accumulo Issue Type: Improvement Reporter: Nick Felts Assignee: Nick Felts The crypto module class and secret key encryption strategy class should each be checked to ensure that if one is set not-null, the other is also enabled. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] milleruntime closed pull request #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache
milleruntime closed pull request #364: ACCUMULO-4778 Initial feedback for table
name to id mapping cache
URL: https://github.com/apache/accumulo/pull/364
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
b/core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
index f5e1fa0499..e7a6d73583 100644
---
a/core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
+++
b/core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
@@ -19,37 +19,26 @@
import static com.google.common.base.Preconditions.checkArgument;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicLong;
import org.apache.accumulo.core.client.AccumuloException;
import org.apache.accumulo.core.client.AccumuloSecurityException;
import org.apache.accumulo.core.client.BatchWriter;
import org.apache.accumulo.core.client.BatchWriterConfig;
-import org.apache.accumulo.core.client.Instance;
import org.apache.accumulo.core.client.MultiTableBatchWriter;
import org.apache.accumulo.core.client.MutationsRejectedException;
import org.apache.accumulo.core.client.TableNotFoundException;
import org.apache.accumulo.core.client.TableOfflineException;
import org.apache.accumulo.core.data.Mutation;
-import org.apache.accumulo.core.master.state.tables.TableState;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
import com.google.common.util.concurrent.UncheckedExecutionException;
public class MultiTableBatchWriterImpl implements MultiTableBatchWriter {
- public static final long DEFAULT_CACHE_TIME = 200;
- public static final TimeUnit DEFAULT_CACHE_TIME_UNIT = TimeUnit.MILLISECONDS;
private static final Logger log =
LoggerFactory.getLogger(MultiTableBatchWriterImpl.class);
private AtomicBoolean closed;
- private AtomicLong cacheLastState;
private class TableBatchWriter implements BatchWriter {
@@ -82,49 +71,17 @@ public void flush() {
}
- /**
- * CacheLoader which will look up the internal table ID for a given table
name.
- */
- private class TableNameToIdLoader extends CacheLoader {
-
-@Override
-public String load(String tableName) throws Exception {
- Instance instance = context.getInstance();
- String tableId = Tables.getNameToIdMap(instance).get(tableName);
-
- if (tableId == null)
-throw new TableNotFoundException(null, tableName, null);
-
- if (Tables.getTableState(instance, tableId) == TableState.OFFLINE)
-throw new TableOfflineException(instance, tableId);
-
- return tableId;
-}
-
- }
-
private TabletServerBatchWriter bw;
private ConcurrentHashMap tableWriters;
private final ClientContext context;
- private final LoadingCache nameToIdCache;
public MultiTableBatchWriterImpl(ClientContext context, BatchWriterConfig
config) {
-this(context, config, DEFAULT_CACHE_TIME, DEFAULT_CACHE_TIME_UNIT);
- }
-
- public MultiTableBatchWriterImpl(ClientContext context, BatchWriterConfig
config, long cacheTime, TimeUnit cacheTimeUnit) {
checkArgument(context != null, "context is null");
checkArgument(config != null, "config is null");
-checkArgument(cacheTimeUnit != null, "cacheTimeUnit is null");
this.context = context;
this.bw = new TabletServerBatchWriter(context, config);
tableWriters = new ConcurrentHashMap<>();
this.closed = new AtomicBoolean(false);
-this.cacheLastState = new AtomicLong(0);
-
-// Potentially up to ~500k used to cache names to IDs with "segments" of
(maybe) ~1000 entries
-nameToIdCache = CacheBuilder.newBuilder().expireAfterWrite(cacheTime,
cacheTimeUnit).concurrencyLevel(10).maximumSize(1).initialCapacity(20)
-.build(new TableNameToIdLoader());
}
@Override
@@ -161,7 +118,7 @@ protected void finalize() {
*/
private String getId(String tableName) throws TableNotFoundException {
try {
- return nameToIdCache.get(tableName);
+ return Tables.getTableId(context.inst, tableName);
} catch (UncheckedExecutionException e) {
Throwable cause = e.getCause();
@@ -176,20 +133,6 @@ private String getId(String tableName) throws
TableNotFoundException {
}
throw e;
-} catch (ExecutionException e) {
- Throwable cause = e.getCause();
-
-
[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_r164869434
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
Review comment:
Thanks!
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164869308
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
Review comment:
Oh I meant tests, all of them together are a good verification of the new
options.
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164868666
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
+ @Test
+ public void testCreateTableWithLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2", true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Set expectedColFams = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+for (Entry entry : lMap.entrySet()) {
+ Assert.assertTrue(entry.getKey().equals("locg1"));
+ Assert.assertTrue(entry.getValue().containsAll(expectedColFams));
+}
+ts.exec("deletetable -f " + table);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one locality group. It is
possible to
+ * set multiple groups if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2 locg2=colfam1",
true);
Review comment:
Neat, I didn't know about this capability of the option parser.
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164863051
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +168,67 @@ 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)));
+ 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(","));
+ if (scopeList.size() > 3) // max of three scope settings allowed
+throw new IllegalArgumentException("Too many scopes supplied");
+ EnumSet scopes =
EnumSet.noneOf(IteratorUtil.IteratorScope.class);
+ if (scopeList.contains("all") || scopeList.contains("scan"))
Review comment:
Will this code ignore an unknown scope? For example if I gave the scopes
`scan,nimc`, I think it would not complain about `nimc`.
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] 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_r164864977
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
Review comment:
Wish i could take credit, but this was already in the test :)
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] 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_r164864624
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +168,67 @@ 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)));
+ localityGroupMap.put(groupName, colFams);
Review comment:
Will add sanity check.
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] 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_r164864370
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +168,67 @@ 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)));
+ 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(","));
+ if (scopeList.size() > 3) // max of three scope settings allowed
+throw new IllegalArgumentException("Too many scopes supplied");
+ EnumSet scopes =
EnumSet.noneOf(IteratorUtil.IteratorScope.class);
+ if (scopeList.contains("all") || scopeList.contains("scan"))
Review comment:
Yep. I missed that. I'll add some error checking to verify the scopes are
one of the acceptable values.
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164863285
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +168,67 @@ 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)));
+ localityGroupMap.put(groupName, colFams);
Review comment:
A sanity check to see if the if the group name already exist in the map
would be nice.
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164863051
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -150,9 +168,67 @@ 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)));
+ 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(","));
+ if (scopeList.size() > 3) // max of three scope settings allowed
+throw new IllegalArgumentException("Too many scopes supplied");
+ EnumSet scopes =
EnumSet.noneOf(IteratorUtil.IteratorScope.class);
+ if (scopeList.contains("all") || scopeList.contains("scan"))
Review comment:
Will this code would ignore an unknown scope? For example if I gave the
scopes `scan,nimc`, I think it would not complain about `nimc`.
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] 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_r164863136
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -174,13 +250,20 @@ public Options getOptions() {
"prevent users from writing data they cannot read. When enabling
this, consider disabling bulk import and alter table.");
createTableOptFormatter = new Option("f", "formatter", true, "default
formatter to set");
createTableOptInitProp = new Option("prop", "init-properties", true, "user
defined initial properties");
-
createTableOptCopyConfig.setArgName("table");
createTableOptCopySplits.setArgName("table");
createTableOptSplit.setArgName("filename");
createTableOptFormatter.setArgName("className");
createTableOptInitProp.setArgName("properties");
+createTableOptLocalityProps = new Option("l", "locality", true, "create
locality groups at table creation");
+createTableOptLocalityProps.setArgName("group=col_fam[,col_fam]");
+createTableOptLocalityProps.setArgs(Option.UNLIMITED_VALUES);
Review comment:
Yes.
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] 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_r164862676
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
+ @Test
+ public void testCreateTableWithLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2", true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Set expectedColFams = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+for (Entry entry : lMap.entrySet()) {
+ Assert.assertTrue(entry.getKey().equals("locg1"));
+ Assert.assertTrue(entry.getValue().containsAll(expectedColFams));
+}
+ts.exec("deletetable -f " + table);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one locality group. It is
possible to
+ * set multiple groups if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2 locg2=colfam1",
true);
Review comment:
The space enables splitting of the parameters so that multiple groups can be
parsed easily. It mimics the format of the setgroups command:
`setgroups ={,}{ ={,}}
[-?] [-t ]`
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] 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_r164862676
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
+ @Test
+ public void testCreateTableWithLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2", true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Set expectedColFams = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+for (Entry entry : lMap.entrySet()) {
+ Assert.assertTrue(entry.getKey().equals("locg1"));
+ Assert.assertTrue(entry.getValue().containsAll(expectedColFams));
+}
+ts.exec("deletetable -f " + table);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one locality group. It is
possible to
+ * set multiple groups if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2 locg2=colfam1",
true);
Review comment:
The space enables splitting of the parameters so that multiple groups can be
parsed easily. It mimics the format of the setgroups command:
setgroups ={,}{ ={,}} [-?]
[-t ].
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] 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_r164861806
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
+ @Test
+ public void testCreateTableWithLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2", true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Set expectedColFams = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+for (Entry entry : lMap.entrySet()) {
+ Assert.assertTrue(entry.getKey().equals("locg1"));
+ Assert.assertTrue(entry.getValue().containsAll(expectedColFams));
+}
+ts.exec("deletetable -f " + table);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one locality group. It is
possible to
+ * set multiple groups if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2 locg2=colfam1",
true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Assert.assertTrue(lMap.keySet().contains("locg1"));
+Assert.assertTrue(lMap.keySet().contains("locg2"));
+Set expectedColFams1 = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+Set expectedColFams2 = new HashSet<>(Arrays.asList(new
Text("colfam1")));
+Assert.assertTrue(lMap.get("locg1").containsAll(expectedColFams1));
+Assert.assertTrue(lMap.get("locg2").containsAll(expectedColFams2));
+ts.exec("deletetable -f " + table);
+ }
+
+ @Test
+ public void testCreateTableWithLocalityGroupsBadArguments() throws
IOException {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1 fam1,fam2", false);
+ts.exec("createtable " + table + "-l", false);
+ts.exec("createtable " + table + " -l locg1 = fam1,fam2", false);
+ts.exec("createtable " + table + " -l locg1=fam1 ,fam2", false);
+ts.exec("createtable " + table + " -l locg1", false);
+ts.exec("createtable " + table + " group=fam1", false);
+ts.exec("createtable " + table + "-l fam1,fam2", false);
+ts.exec("createtable " + table + " -local locg1=fam1,fam2", false);
+ }
+
+ @Test
+ public void testCreateTableWithIterators() throws Exception {
+final String tmpTable = "tmpTable";
+final String table = name.getMethodName();
+
+// create iterator profile
+// Will use tmpTable for creating profile since setshelliter is requiring
a table
+// even though its command line help indicates that it is optional. Likely
due to
+// the fact that setshelliter extends setiter, which does require a table
argument.
+ts.exec("createtable " + tmpTable, true);
+String output = ts.exec("tables");
+Assert.assertTrue(output.contains(tmpTable));
+
+ts.input.set("\n5000\n\n");
+ts.exec("setshelliter -n itname -p 10 -pn profile1 -ageoff -t " +
tmpTable, true);
+output = ts.exec("listshelliter");
+Assert.assertTrue(output.contains("Profile : profile1"));
+
+// create table making use of the iterator profile
+ts.exec("createtable " + table + " -i profile1:scan,minc", true);
+ts.exec("insert foo a b c", true);
+ts.exec("scan", true, "foo a:b []c");
+ts.exec("sleep 6", true);
+ts.exec("scan", true, "", true);
+ts.exec("deletetable -f " + table);
+ts.exec("deletetable -f " + tmpTable);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one iterator upon table
creation. It is
+ * possible to set multiple if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleIterators() throws Exception {
+final String tmpTable = "tmpTable";
+final String table = name.getMethodName();
+
+// create iterator profile
+// Will use tmpTable for creating profile since setshelliter is requiring
a table
Review comment:
I agree. I created a ticket for it this morning
(https://issues.apache.org/jira/browse/ACCUMULO-4791)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and
[GitHub] keith-turner commented on a change in pull request #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164861376
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
+ @Test
+ public void testCreateTableWithLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2", true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Set expectedColFams = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+for (Entry entry : lMap.entrySet()) {
+ Assert.assertTrue(entry.getKey().equals("locg1"));
+ Assert.assertTrue(entry.getValue().containsAll(expectedColFams));
+}
+ts.exec("deletetable -f " + table);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one locality group. It is
possible to
+ * set multiple groups if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2 locg2=colfam1",
true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Assert.assertTrue(lMap.keySet().contains("locg1"));
+Assert.assertTrue(lMap.keySet().contains("locg2"));
+Set expectedColFams1 = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+Set expectedColFams2 = new HashSet<>(Arrays.asList(new
Text("colfam1")));
+Assert.assertTrue(lMap.get("locg1").containsAll(expectedColFams1));
+Assert.assertTrue(lMap.get("locg2").containsAll(expectedColFams2));
+ts.exec("deletetable -f " + table);
+ }
+
+ @Test
+ public void testCreateTableWithLocalityGroupsBadArguments() throws
IOException {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1 fam1,fam2", false);
+ts.exec("createtable " + table + "-l", false);
+ts.exec("createtable " + table + " -l locg1 = fam1,fam2", false);
+ts.exec("createtable " + table + " -l locg1=fam1 ,fam2", false);
+ts.exec("createtable " + table + " -l locg1", false);
+ts.exec("createtable " + table + " group=fam1", false);
+ts.exec("createtable " + table + "-l fam1,fam2", false);
+ts.exec("createtable " + table + " -local locg1=fam1,fam2", false);
+ }
+
+ @Test
+ public void testCreateTableWithIterators() throws Exception {
+final String tmpTable = "tmpTable";
+final String table = name.getMethodName();
+
+// create iterator profile
+// Will use tmpTable for creating profile since setshelliter is requiring
a table
+// even though its command line help indicates that it is optional. Likely
due to
+// the fact that setshelliter extends setiter, which does require a table
argument.
+ts.exec("createtable " + tmpTable, true);
+String output = ts.exec("tables");
+Assert.assertTrue(output.contains(tmpTable));
+
+ts.input.set("\n5000\n\n");
+ts.exec("setshelliter -n itname -p 10 -pn profile1 -ageoff -t " +
tmpTable, true);
+output = ts.exec("listshelliter");
+Assert.assertTrue(output.contains("Profile : profile1"));
+
+// create table making use of the iterator profile
+ts.exec("createtable " + table + " -i profile1:scan,minc", true);
+ts.exec("insert foo a b c", true);
+ts.exec("scan", true, "foo a:b []c");
+ts.exec("sleep 6", true);
+ts.exec("scan", true, "", true);
+ts.exec("deletetable -f " + table);
+ts.exec("deletetable -f " + tmpTable);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one iterator upon table
creation. It is
+ * possible to set multiple if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleIterators() throws Exception {
+final String tmpTable = "tmpTable";
+final String table = name.getMethodName();
+
+// create iterator profile
+// Will use tmpTable for creating profile since setshelliter is requiring
a table
Review comment:
It would be nice to look into fixing this.
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
[GitHub] keith-turner commented on a change in pull request #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164861137
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
Review comment:
Very nice test!!
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164861047
##
File path:
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
##
@@ -174,13 +250,20 @@ public Options getOptions() {
"prevent users from writing data they cannot read. When enabling
this, consider disabling bulk import and alter table.");
createTableOptFormatter = new Option("f", "formatter", true, "default
formatter to set");
createTableOptInitProp = new Option("prop", "init-properties", true, "user
defined initial properties");
-
createTableOptCopyConfig.setArgName("table");
createTableOptCopySplits.setArgName("table");
createTableOptSplit.setArgName("filename");
createTableOptFormatter.setArgName("className");
createTableOptInitProp.setArgName("properties");
+createTableOptLocalityProps = new Option("l", "locality", true, "create
locality groups at table creation");
+createTableOptLocalityProps.setArgName("group=col_fam[,col_fam]");
+createTableOptLocalityProps.setArgs(Option.UNLIMITED_VALUES);
Review comment:
Does this enables parsing of the space separated values?
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 #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
keith-turner 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_r164860539
##
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##
@@ -2020,4 +2022,143 @@ public void testSummarySelection() throws Exception {
// check that there are two files, with none having extra summary info
assertMatches(output,
"(?sm).*^.*total[:]2[,]\\s+missing[:]0[,]\\s+extra[:]0.*$.*");
}
+
+ @Test
+ public void testCreateTableWithLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2", true);
+Connector connector = getConnector();
+Map lMap =
connector.tableOperations().getLocalityGroups(table);
+Set expectedColFams = new HashSet<>(Arrays.asList(new Text("fam1"),
new Text("fam2")));
+for (Entry entry : lMap.entrySet()) {
+ Assert.assertTrue(entry.getKey().equals("locg1"));
+ Assert.assertTrue(entry.getValue().containsAll(expectedColFams));
+}
+ts.exec("deletetable -f " + table);
+ }
+
+ /**
+ * Due to the existing complexity of the createtable command, the
createtable help only displays an example of setting one locality group. It is
possible to
+ * set multiple groups if needed. This test verifies that capability.
+ */
+ @Test
+ public void testCreateTableWithMultipleLocalityGroups() throws Exception {
+final String table = name.getMethodName();
+ts.exec("createtable " + table + " -l locg1=fam1,fam2 locg2=colfam1",
true);
Review comment:
I am surprised the space between `fam2` and `logg2=` does not throw things
off.
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 # 1002 - Fixed
The Apache Jenkins build system has built Accumulo-Pull-Requests (build #1002) Status: Fixed Check console output at https://builds.apache.org/job/Accumulo-Pull-Requests/1002/ to view the results.
[GitHub] jmark99 commented on issue #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
jmark99 commented on issue #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods URL: https://github.com/apache/accumulo/pull/370#issuecomment-361684904 Yep, I updated the createtable help usage to display the new options. 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 issue #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
milleruntime commented on issue #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods URL: https://github.com/apache/accumulo/pull/370#issuecomment-361684413 Looks good to me. Does the command line help print the new options? I've worked with the shell some but can't remember how it prints the usage. 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 issue #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache
milleruntime commented on issue #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache URL: https://github.com/apache/accumulo/pull/364#issuecomment-361670549 For future reference, I pushed the tests I created to my accumulo-benchmark repo: https://github.com/milleruntime/accumulo-benchmark/blob/master/jmh-test/src/main/java/org/apache/accumulo/TablesBenchmark.java 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 issue #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache
milleruntime commented on issue #364: ACCUMULO-4778 Initial feedback for table name to id mapping cache URL: https://github.com/apache/accumulo/pull/364#issuecomment-361670029 Running the benchmark tests with multiple threads only shows a greater improvement. I think this is ready to merge. 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
[jira] [Updated] (ACCUMULO-4772) Update Accumulo shell to utilize new NewTableConfiguration methods
[ https://issues.apache.org/jira/browse/ACCUMULO-4772?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] ASF GitHub Bot updated ACCUMULO-4772: - Labels: pull-request-available (was: ) > Update Accumulo shell to utilize new NewTableConfiguration methods > -- > > Key: ACCUMULO-4772 > URL: https://issues.apache.org/jira/browse/ACCUMULO-4772 > Project: Accumulo > Issue Type: Improvement > Components: shell >Reporter: Mark Owens >Assignee: Mark Owens >Priority: Minor > Labels: pull-request-available > Fix For: 2.0.0 > > > ACCUMULO-4732 adds the capability for NewTableConfiguration to preconfigure > iterators and locality groups prior to table creation. Update the Accumulo > shell to allow the same capability. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[GitHub] jmark99 opened a new pull request #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods
jmark99 opened a new pull request #370: ACCUMULO-4772 Update shell to use NewTableConfiguration methods URL: https://github.com/apache/accumulo/pull/370 ACCUMULO-4772 Update Accumulo shell to utilize new NewTableConfiguration methods Added capability to pre-configure iterators and locality groups when creating new tables via the Accumulo shell. Updated CreateTableCommand.java to accept locality group and iterator data. Updated ShellServerIT.java to test the new capabilities. 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
[jira] [Created] (ACCUMULO-4791) setshelliter command has incorrect usage statement
Mark Owens created ACCUMULO-4791: Summary: setshelliter command has incorrect usage statement Key: ACCUMULO-4791 URL: https://issues.apache.org/jira/browse/ACCUMULO-4791 Project: Accumulo Issue Type: Bug Components: shell Reporter: Mark Owens Fix For: 1.9.0, 2.0.0 When using the Accumulo shell, the setshelliter command should not require a table name and the usage indicates that a table name is optional, as expected. But the command will fail unless an existing table name is supplied. Most likely due to the fact that setshelliter extends setiter, which does require a valid table name. The setshelliter command should be updated to work as the current usage indicates, i.e., no table name should be required. -- This message was sent by Atlassian JIRA (v7.6.3#76005)
