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 2021/06/23 12:13:27 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2177: Fix VerifySerialRecoveryIT failure
milleruntime opened a new pull request #2177:
URL: https://github.com/apache/accumulo/pull/2177
* Fixes #2173 failure with the decoding of row bytes
* Adds tests to LogFileKeyTest
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2177: Adds tests to LogFileKeyTest
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2177:
URL: https://github.com/apache/accumulo/pull/2177#discussion_r657286790
##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/LogFileKeyTest.java
##########
@@ -98,4 +103,48 @@ public void testSortOrder() {
}
}
+
+ private void testToFromKey(int tabletId, long seq) throws IOException {
+ var logFileKey = nk(LogEvents.DEFINE_TABLET, tabletId, seq);
Review comment:
I think the different events and sorting get covered enough in `SortedLogRecoveryTest`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2177: Adds tests to LogFileKeyTest
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2177:
URL: https://github.com/apache/accumulo/pull/2177#discussion_r657201788
##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/LogFileKeyTest.java
##########
@@ -98,4 +103,48 @@ public void testSortOrder() {
}
}
+
+ private void testToFromKey(int tabletId, long seq) throws IOException {
+ var logFileKey = nk(LogEvents.DEFINE_TABLET, tabletId, seq);
+ logFileKey.tablet = new KeyExtent(TableId.of("5"), new Text("b"), null);
+ Key k = logFileKey.toKey();
+
+ var converted = LogFileKey.fromKey(k);
Review comment:
The use of `var` here obscures the types, which are not otherwise obvious. This doesn't need to be changed, but it's probably not the best use of `var`.
##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/LogFileKeyTest.java
##########
@@ -98,4 +103,48 @@ public void testSortOrder() {
}
}
+
+ private void testToFromKey(int tabletId, long seq) throws IOException {
+ var logFileKey = nk(LogEvents.DEFINE_TABLET, tabletId, seq);
+ logFileKey.tablet = new KeyExtent(TableId.of("5"), new Text("b"), null);
+ Key k = logFileKey.toKey();
+
+ var converted = LogFileKey.fromKey(k);
+ assertEquals("Failed to convert tabletId = " + tabletId + " seq = " + seq, logFileKey,
+ converted);
+ }
+
+ @Test
+ public void convertToKeyLargeTabletId() throws IOException {
+ // test continuous range of numbers to make sure all cases of signed bytes are covered
+ for (int i = -1_000; i < 1_000_000; i++) {
+ testToFromKey(i, 1);
+ }
+ }
+
+ @Test
+ public void convertToKeyLargeSeq() throws IOException {
+ // use numbers large enough to cover all bytes used
+ testToFromKey(1, 8); // 1 byte
+ testToFromKey(1, 1_000L); // 2 bytes
+ testToFromKey(1, 1_222_333L); // 3 bytes
+ testToFromKey(1, 100_000_000L); // 4 bytes
+ testToFromKey(1, 111_222_333_444L); // 5 bytes
+ testToFromKey(1, 1_000_222_333_777L); // 6 bytes
+ testToFromKey(1, 5_222_000_222_333_777L); // 7 bytes
+ testToFromKey(1, 500_222_333_444_555_666L); // 8 bytes
+
+ // test with more tabletId bytes
+ testToFromKey(-1000, 500_222_333_444_555_666L);
+ testToFromKey(10_000_000, 500_222_333_444_555_666L);
+ testToFromKey(Integer.MAX_VALUE, Long.MAX_VALUE);
+ testToFromKey(Integer.MIN_VALUE, Long.MAX_VALUE);
+ testToFromKey(Integer.MIN_VALUE, 0);
+ testToFromKey(Integer.MAX_VALUE, 0);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testNegativeSeq() throws IOException {
+ testToFromKey(1, -1);
+ }
Review comment:
Consider using `assertThrows` in future. You can do useful things, like return the thrown exception for additional verification, as in:
```java
var e = assertThrows(IllegalArgumentException.class, () -> testToFromKey(1, -1));
assertEquals("expected exception message", e.getMessage());
assertTrue(e.getCause() instanceof SomeCauseExceptionType);
```
It also gives you more control over checking which specific line throws the exception, rather than just checking that it's thrown at *some* point in the method's execution.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #2177: Adds tests to LogFileKeyTest
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #2177:
URL: https://github.com/apache/accumulo/pull/2177
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2177: Adds tests to LogFileKeyTest
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2177:
URL: https://github.com/apache/accumulo/pull/2177#discussion_r657264184
##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/LogFileKeyTest.java
##########
@@ -98,4 +103,48 @@ public void testSortOrder() {
}
}
+
+ private void testToFromKey(int tabletId, long seq) throws IOException {
+ var logFileKey = nk(LogEvents.DEFINE_TABLET, tabletId, seq);
Review comment:
Would it make sense to loop through all of the LogEvent types here, running the test for each one?
##########
File path: server/tserver/src/test/java/org/apache/accumulo/tserver/log/LogFileKeyTest.java
##########
@@ -98,4 +103,48 @@ public void testSortOrder() {
}
}
+
+ private void testToFromKey(int tabletId, long seq) throws IOException {
+ var logFileKey = nk(LogEvents.DEFINE_TABLET, tabletId, seq);
+ logFileKey.tablet = new KeyExtent(TableId.of("5"), new Text("b"), null);
+ Key k = logFileKey.toKey();
+
+ var converted = LogFileKey.fromKey(k);
+ assertEquals("Failed to convert tabletId = " + tabletId + " seq = " + seq, logFileKey,
+ converted);
+ }
+
+ @Test
+ public void convertToKeyLargeTabletId() throws IOException {
+ // test continuous range of numbers to make sure all cases of signed bytes are covered
+ for (int i = -1_000; i < 1_000_000; i++) {
+ testToFromKey(i, 1);
+ }
+ }
+
+ @Test
+ public void convertToKeyLargeSeq() throws IOException {
+ // use numbers large enough to cover all bytes used
+ testToFromKey(1, 8); // 1 byte
+ testToFromKey(1, 1_000L); // 2 bytes
Review comment:
could use hex instead of decimal, would make it easier to see what bytes are covered.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org