You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/04/22 20:46:06 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request, #2648: Address TODOs found in code
DomGarguilo opened a new pull request, #2648:
URL: https://github.com/apache/accumulo/pull/2648
I looked through some of the TODOs in the comments and did my best to address those that I could.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2648: Address TODOs found in code
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2648:
URL: https://github.com/apache/accumulo/pull/2648#discussion_r857245684
##########
test/src/main/java/org/apache/accumulo/test/ConditionalWriterIT.java:
##########
@@ -883,15 +884,19 @@ public void testBigBatch() throws Exception {
int count = 0;
- // TODO check got each row back
+ Set<String> rowsReceived = new HashSet<>();
while (results.hasNext()) {
Result result = results.next();
+ rowsReceived.add(new String(result.getMutation().getRow()));
Review Comment:
Should explicitly use `UTF_8` when constructing a String from byte array, otherwise, this could fail on some platforms where `UTF_8` is not the default.
##########
shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java:
##########
@@ -236,4 +239,14 @@ public Properties getClientProperties() {
return props;
}
+ static class PositiveInteger implements IParameterValidator {
+ public void validate(String name, String value) throws ParameterException {
Review Comment:
```suggestion
@Override
public void validate(String name, String value) throws ParameterException {
```
##########
test/src/main/java/org/apache/accumulo/test/ConditionalWriterIT.java:
##########
@@ -883,15 +884,19 @@ public void testBigBatch() throws Exception {
int count = 0;
- // TODO check got each row back
+ Set<String> rowsReceived = new HashSet<>();
while (results.hasNext()) {
Result result = results.next();
+ rowsReceived.add(new String(result.getMutation().getRow()));
assertEquals(Status.ACCEPTED, result.getStatus());
count++;
}
assertEquals(num, count, "Did not receive the expected number of results");
+ Set<String> rowsExpected = rows.stream().map(String::new).collect(Collectors.toSet());
Review Comment:
Should explicitly use `UTF_8` when constructing new strings here.
##########
shell/src/main/java/org/apache/accumulo/shell/ShellOptionsJC.java:
##########
@@ -236,4 +239,14 @@ public Properties getClientProperties() {
return props;
}
+ static class PositiveInteger implements IParameterValidator {
+ public void validate(String name, String value) throws ParameterException {
+ int n = Integer.parseInt(value);
+ if (n < 0) {
+ throw new ParameterException(
+ "Parameter " + name + " should be positive (found " + value + ")");
+ }
Review Comment:
```suggestion
int n = -1;
try {
n = Integer.parseInt(value);
} catch (NumberFormatException e) {
// ignore, will be handled below
}
if (n < 0) {
throw new ParameterException(
"Parameter " + name + " should be a positive integer (was " + value + ")");
}
```
##########
test/src/main/java/org/apache/accumulo/test/ExistingMacIT.java:
##########
@@ -88,20 +88,21 @@ private void createEmptyConfig(File confFile) throws IOException {
@Test
public void testExistingInstance() throws Exception {
+ final String rootUser = "root";
AccumuloClient client =
- getCluster().createAccumuloClient("root", new PasswordToken(ROOT_PASSWORD));
+ getCluster().createAccumuloClient(rootUser, new PasswordToken(ROOT_PASSWORD));
- client.tableOperations().create("table1");
+ final String table1 = "table1";
Review Comment:
This table name should be unique for this test method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #2648: Address TODOs found in code
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #2648:
URL: https://github.com/apache/accumulo/pull/2648
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org