You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ddanielr (via GitHub)" <gi...@apache.org> on 2024/03/07 20:21:10 UTC
[PR] Throw error when non-standard chars exist [accumulo]
ddanielr opened a new pull request, #4348:
URL: https://github.com/apache/accumulo/pull/4348
Adds an explicit error message when base64 coding should have been used.
Adds override flag to generate human-readable output.
closes: 4347
I'm not sold on the `--human-readable` flag name. `--force` could be used instead.
--
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
Re: [PR] Throw error when non-standard chars exist [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4348:
URL: https://github.com/apache/accumulo/pull/4348#discussion_r1519956189
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/GenerateSplits.java:
##########
@@ -265,7 +272,13 @@ private static String encode(boolean encode, Text text) {
} else if (c >= 32 && c <= 126) {
sb.append((char) c);
} else {
- log.debug("Dropping non printable char: \\x{}", Integer.toHexString(c));
+ String errMsg = "non printable char: \\x" + Integer.toHexString(c);
+ // Fail if non-printable characters are detected.
+ if (!humanRead) {
+ throw new UnsupportedOperationException(
+ errMsg + " detected. Must use Base64 encoded output");
+ }
+ log.info("Dropping {}", errMsg);
Review Comment:
Replaced this code with logic that if it detects a non-standard char it throws the exception.
--
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
Re: [PR] Throw error when non-standard chars exist [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4348:
URL: https://github.com/apache/accumulo/pull/4348#discussion_r1519965774
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/GenerateSplits.java:
##########
@@ -267,18 +280,12 @@ private static String encode(boolean encode, Text text, boolean humanRead) {
StringBuilder sb = new StringBuilder();
for (byte aByte : bytes) {
int c = 0xff & aByte;
- if (c == '\\') {
- sb.append("\\\\");
- } else if (c >= 32 && c <= 126) {
+ if (allowedChars.contains((char) c)) {
sb.append((char) c);
} else {
- String errMsg = "non printable char: \\x" + Integer.toHexString(c);
// Fail if non-printable characters are detected.
- if (!humanRead) {
- throw new UnsupportedOperationException(
- errMsg + " detected. Must use Base64 encoded output");
- }
- log.info("Dropping {}", errMsg);
+ throw new UnsupportedOperationException("Non printable char: \\x" + Integer.toHexString(c)
+ + " detected. Must use Base64 encoded output");
Review Comment:
```suggestion
+ " detected. Must use Base64 encoded output. The behavior around non printable chars changed in 2.1.3 to throw an error, the previous behavior was likely to cause bugs.");
```
--
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
Re: [PR] Throw error when non-standard chars exist [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4348:
URL: https://github.com/apache/accumulo/pull/4348#discussion_r1518290448
##########
core/src/test/java/org/apache/accumulo/core/file/rfile/GenerateSplitsTest.java:
##########
@@ -156,6 +169,49 @@ public void testEvenlySpaced() {
assertEquals(Set.of("001", "002", "003", "004", "005", "006", "007", "008", "009"), desired);
}
+ @Test
+ public void testNullValues() throws Exception {
+ trf.openWriter(false);
+ trf.writer.startNewLocalityGroup("lg1", newColFamByteSequence("cf1", "cf2"));
+ trf.writer.append(newKey("r1\0a", "cf1", "cq1", "L1", 55), newValue("foo1"));
+ trf.writer.append(newKey("r2\0b", "cf2", "cq1", "L1", 55), newValue("foo2"));
+ trf.writer.append(newKey("r3\0c", "cf2", "cq1", "L1", 55), newValue("foo3"));
+ trf.writer.startNewLocalityGroup("lg2", newColFamByteSequence("cf3", "cf4"));
+ trf.writer.append(newKey("r4\0d", "cf3", "cq1", "L1", 55), newValue("foo4"));
+ trf.writer.append(newKey("r5\0e", "cf4", "cq1", "L1", 55), newValue("foo5"));
+ trf.writer.append(newKey("r6\0f", "cf4", "cq1", "L1", 55), newValue("foo6"));
+ trf.closeWriter();
+
+ File file = new File(tempDir, "testGenerateSplitsWithNulls.rf");
+ assertTrue(file.createNewFile(), "Failed to create file: " + file);
+ try (var fileOutputStream = new FileOutputStream(file)) {
+ fileOutputStream.write(trf.baos.toByteArray());
+ }
+ rfilePath = "file:" + file.getAbsolutePath();
+ log.info("Wrote to file {}", rfilePath);
+
+ File splitsFile = new File(tempDir, "testSplitsFileWithNulls");
+ assertTrue(splitsFile.createNewFile(), "Failed to create file: " + splitsFile);
+ splitsFilePath = splitsFile.getAbsolutePath();
+
+ List<String> args = List.of(rfilePath, "--num", "2", "-sf", splitsFilePath);
+
+ try {
+ GenerateSplits.main(args.toArray(new String[0]));
+ } catch (Exception e) {
Review Comment:
May be able to use junit assertThrows here.
--
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
Re: [PR] Throw error when non-standard chars exist [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr merged PR #4348:
URL: https://github.com/apache/accumulo/pull/4348
--
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
Re: [PR] Throw error when non-standard chars exist [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4348:
URL: https://github.com/apache/accumulo/pull/4348#discussion_r1519634152
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/GenerateSplits.java:
##########
@@ -265,7 +272,13 @@ private static String encode(boolean encode, Text text) {
} else if (c >= 32 && c <= 126) {
sb.append((char) c);
} else {
- log.debug("Dropping non printable char: \\x{}", Integer.toHexString(c));
+ String errMsg = "non printable char: \\x" + Integer.toHexString(c);
+ // Fail if non-printable characters are detected.
+ if (!humanRead) {
+ throw new UnsupportedOperationException(
+ errMsg + " detected. Must use Base64 encoded output");
+ }
+ log.info("Dropping {}", errMsg);
Review Comment:
I think that might be best. Piping the output to `base64 -d` is easy enough and moves the printing requirement to a more supported utility.
I agree with the current questionable handing of '\\'. I'm going to make another change and move this out of draft.
--
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
Re: [PR] Throw error when non-standard chars exist [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #4348:
URL: https://github.com/apache/accumulo/pull/4348#discussion_r1519634152
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/GenerateSplits.java:
##########
@@ -265,7 +272,13 @@ private static String encode(boolean encode, Text text) {
} else if (c >= 32 && c <= 126) {
sb.append((char) c);
} else {
- log.debug("Dropping non printable char: \\x{}", Integer.toHexString(c));
+ String errMsg = "non printable char: \\x" + Integer.toHexString(c);
+ // Fail if non-printable characters are detected.
+ if (!humanRead) {
+ throw new UnsupportedOperationException(
+ errMsg + " detected. Must use Base64 encoded output");
+ }
+ log.info("Dropping {}", errMsg);
Review Comment:
I think that might be best. Piping the output to `base64 -d` is easy enough and moves the printing requirement to a more supported utility.
I agree with the current questionable handing of '\'. I'm going to make another change and move this out of draft.
--
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
Re: [PR] Throw error when non-standard chars exist [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4348:
URL: https://github.com/apache/accumulo/pull/4348#discussion_r1518289149
##########
core/src/main/java/org/apache/accumulo/core/file/rfile/GenerateSplits.java:
##########
@@ -265,7 +272,13 @@ private static String encode(boolean encode, Text text) {
} else if (c >= 32 && c <= 126) {
sb.append((char) c);
} else {
- log.debug("Dropping non printable char: \\x{}", Integer.toHexString(c));
+ String errMsg = "non printable char: \\x" + Integer.toHexString(c);
+ // Fail if non-printable characters are detected.
+ if (!humanRead) {
+ throw new UnsupportedOperationException(
+ errMsg + " detected. Must use Base64 encoded output");
+ }
+ log.info("Dropping {}", errMsg);
Review Comment:
Since the default behavior is being changed, could simplify this code and not have the human readable flag. Just always fail and guide towards using the base64 option.
```suggestion
String errMsg = "non printable char: \\x" + Integer.toHexString(c);
// Fail if non-printable characters are detected.
throw new UnsupportedOperationException(
errMsg + " detected. Must use Base64 encoded output");
```
Unrelated to these changes, but handing of `\` a few lines up seems a bit sketchy.
--
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