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/11/05 19:13:36 UTC
[GitHub] [accumulo-testing] Manno15 opened a new pull request, #254: Refactor logging in Verify for RW Bulk
Manno15 opened a new pull request, #254:
URL: https://github.com/apache/accumulo-testing/pull/254
Closes #239.
Instead of throwing an exception at each possible failure, an error log is recorded instead. Currently, have it to break out of each loop but we could instead complete the loops and log each possible error. Need to do more testing there to see if that would be worthwhile.
--
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-testing] Manno15 commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1014735710
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,36 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ break;
Review Comment:
I forgot to add these. and the ones below but yes, they should. I will do more testing on the breaks, but it should match the same behavior it previously did there. As I mentioned in the description, we could forgo the breaks and then log everything if that is the desired behavior.
--
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-testing] Manno15 commented on pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#issuecomment-1320963290
Tests are failing due to the version change of accumulo. I will look into fixing it tomorrow. This PR should be ready for review still.
--
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-testing] Manno15 commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1014735557
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -67,12 +67,15 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
String user = env.getAccumuloClient().whoami();
Authorizations auths = env.getAccumuloClient().securityOperations().getUserAuthorizations(user);
RowIterator rowIter;
+ Boolean errorFound = false;
Review Comment:
Whoops. Will fix.
--
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-testing] ctubbsii commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1014726242
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -67,12 +67,15 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
String user = env.getAccumuloClient().whoami();
Authorizations auths = env.getAccumuloClient().securityOperations().getUserAuthorizations(user);
RowIterator rowIter;
+ Boolean errorFound = false;
Review Comment:
```suggestion
boolean errorFound = false;
```
--
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-testing] Manno15 commented on pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#issuecomment-1304815688
https://github.com/apache/accumulo-testing/pull/254/commits/f5ff88e2130e57be61d7a4a25f54f4dbe7c3b064 addresses the comments.
--
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-testing] keith-turner commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1027135992
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,39 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ errorFound = true;
+ break;
Review Comment:
> @keith-turner Any thoughts on this and what approach I should take?
Removing all breaks seems like a good way to go, as long as what it logs is accurate. I think its ok if it logs a bunch. Its good to see all of the problems in the data.
> It could also affect the outer if statment since prev is never assigned the value of curr which happens at the end of the inner while loop
I agree the breaks in the inner loop could cause the check following the inner loop to report a false positive.
--
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-testing] keith-turner commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1014730163
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,36 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ break;
+ }
- if (!entry.getValue().toString().equals("1"))
- throw new Exception("Bad marker value " + entry.getKey() + " " + entry.getValue());
+ if (!entry.getValue().toString().equals("1")) {
+ log.error("Bad marker value for row {} {}.\n Value expected to be one", entry.getKey(),
+ entry.getValue());
+ break;
Review Comment:
```suggestion
errorFound=true;
break;
```
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,36 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ break;
Review Comment:
Will breaking stop it from printing other problems in the test data?
Should this set errorFound?
```suggestion
errorFound=true;
break;
```
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,36 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ break;
+ }
- if (!entry.getValue().toString().equals("1"))
- throw new Exception("Bad marker value " + entry.getKey() + " " + entry.getValue());
+ if (!entry.getValue().toString().equals("1")) {
+ log.error("Bad marker value for row {} {}.\n Value expected to be one", entry.getKey(),
+ entry.getValue());
+ break;
+ }
prev = curr;
}
if (BulkPlusOne.counter.get() != prev) {
- throw new Exception("Row " + rowText + " does not have all markers "
- + BulkPlusOne.counter.get() + " " + prev);
+ log.error("Row {} does not have all markers. Current marker: {}, Previous marker:{}",
+ rowText, BulkPlusOne.counter.get(), prev);
+ break;
Review Comment:
```suggestion
errorFound=true;
break;
```
--
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-testing] Manno15 commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1014870543
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,39 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ errorFound = true;
+ break;
Review Comment:
As it stands, this break will be hit and break out of the inner while loop and avoid both of the next checks since `prev` will never be reassigned value to `curr` which is at the end of the inner while loop. Removing both `breaks` in the inner loop would hit both checks but unsure of many redundant errors would get logged, if any.
--
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-testing] Manno15 commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1027138991
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,39 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ errorFound = true;
+ break;
Review Comment:
Done in https://github.com/apache/accumulo-testing/pull/254/commits/4cf04bc78bbeee8f3011ac60e20eda610876c5a4
--
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-testing] Manno15 commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1027138991
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,39 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ errorFound = true;
+ break;
Review Comment:
I will make these changes
--
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-testing] keith-turner commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
keith-turner commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1027135992
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,39 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ errorFound = true;
+ break;
Review Comment:
> @keith-turner Any thoughts on this and what approach I should take?
Removing all breaks seems like a good way to go, as long as what it logs is accurate I think its ok if it logs a bunch. Its good to see all of the problems in the data.
> It could also affect the outer if statment since prev is never assigned the value of curr which happens at the end of the inner while loop
I agree the breaks in the inner loop could cause the check following the inner loop to report a false positive.
--
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-testing] Manno15 commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1027133988
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,39 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ errorFound = true;
+ break;
Review Comment:
@keith-turner Any thoughts on this and what approach I should take?
--
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-testing] Manno15 merged pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 merged PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254
--
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-testing] Manno15 commented on a diff in pull request #254: Refactor logging in Verify for RW Bulk
Posted by GitBox <gi...@apache.org>.
Manno15 commented on code in PR #254:
URL: https://github.com/apache/accumulo-testing/pull/254#discussion_r1014870543
##########
src/main/java/org/apache/accumulo/testing/randomwalk/bulk/Verify.java:
##########
@@ -87,26 +90,39 @@ public void visit(State state, RandWalkEnv env, Properties props) throws Excepti
while (row.hasNext()) {
Entry<Key,Value> entry = row.next();
- if (rowText == null)
+ if (rowText == null) {
rowText = entry.getKey().getRow();
+ }
long curr = Long.parseLong(entry.getKey().getColumnQualifier().toString());
- if (curr - 1 != prev)
- throw new Exception(
- "Bad marker count " + entry.getKey() + " " + entry.getValue() + " " + prev);
+ if (curr - 1 != prev) {
+ log.error("Bad market count. Current row: {} {}, Previous row marker: {}",
+ entry.getKey(), entry.getValue(), prev);
+ errorFound = true;
+ break;
Review Comment:
As it stands, this break will be hit and break out of the inner while loop and avoid the next if statement. It could also affect the outer if statment since `prev` is never assigned the value of `curr` which happens at the end of the inner while loop. Removing both `breaks` in the inner loop would hit both checks but unsure of many redundant errors would get logged, if any.
--
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