ctubbsii commented on code in PR #3640:
URL: https://github.com/apache/accumulo/pull/3640#discussion_r1293789372
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MergeInfo.java:
##########
@@ -84,13 +84,22 @@ public boolean isDelete() {
public boolean needsToBeChopped(KeyExtent otherExtent) {
// During a delete, the block after the merge will be stretched to cover
the deleted area.
// Therefore, it needs to be chopped
- if (!otherExtent.tableId().equals(extent.tableId())) {
- return false;
- }
- if (isDelete()) {
+ // if (!otherExtent.tableId().equals(extent.tableId())) {
+ // return false;
+ // }
+ // if (isDelete()) {
+ // return otherExtent.prevEndRow() != null &&
otherExtent.prevEndRow().equals(extent.endRow());
+ // } else {
+ // return this.extent.overlaps(otherExtent);
+ // }
Review Comment:
What case is this covering that is no longer needed? Could remove commented
out code, would make it easier to review. I know this is WIP, but it still
would be easier.
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/MergeInfoTest.java:
##########
@@ -89,28 +89,29 @@ public void testSerialization() throws Exception {
assertSame(MergeInfo.Operation.DELETE, mi.getOperation());
}
- @Test
- public void testNeedsToBeChopped_DifferentTables() {
- expect(keyExtent.tableId()).andReturn(TableId.of("table1"));
- replay(keyExtent);
- KeyExtent keyExtent2 = createMock(KeyExtent.class);
- expect(keyExtent2.tableId()).andReturn(TableId.of("table2"));
- replay(keyExtent2);
- mi = new MergeInfo(keyExtent, MergeInfo.Operation.MERGE);
- assertFalse(mi.needsToBeChopped(keyExtent2));
- }
-
- @Test
- public void testNeedsToBeChopped_NotDelete() {
- expect(keyExtent.tableId()).andReturn(TableId.of("table1"));
- KeyExtent keyExtent2 = createMock(KeyExtent.class);
- expect(keyExtent2.tableId()).andReturn(TableId.of("table1"));
- replay(keyExtent2);
- expect(keyExtent.overlaps(keyExtent2)).andReturn(true);
- replay(keyExtent);
- mi = new MergeInfo(keyExtent, MergeInfo.Operation.MERGE);
- assertTrue(mi.needsToBeChopped(keyExtent2));
- }
+ // Temporarily comment tests so build doesn't fail
Review Comment:
There is an `@Disabled` JUnit 5 annotation (JUnit4 called it `@Ignore`) to
temporarily disable a test case with an annotated reason.
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/MergeInfoTest.java:
##########
@@ -213,14 +214,14 @@ public void testNeedsToBeChopped() {
assertFalse(info.needsToBeChopped(ke("y", "c", "b")));
assertFalse(info.needsToBeChopped(ke("x", "c", "bb")));
assertFalse(info.needsToBeChopped(ke("x", "b", "a")));
- info = new MergeInfo(ke("x", "b", "a"), MergeInfo.Operation.MERGE);
- assertTrue(info.needsToBeChopped(ke("x", "c", "a")));
- assertTrue(info.needsToBeChopped(ke("x", "aa", "a")));
- assertTrue(info.needsToBeChopped(ke("x", null, null)));
- assertFalse(info.needsToBeChopped(ke("x", "c", "b")));
- assertFalse(info.needsToBeChopped(ke("y", "c", "b")));
- assertFalse(info.needsToBeChopped(ke("x", "c", "bb")));
- assertTrue(info.needsToBeChopped(ke("x", "b", "a")));
+ // info = new MergeInfo(ke("x", "b", "a"), MergeInfo.Operation.MERGE);
+ // assertTrue(info.needsToBeChopped(ke("x", "c", "a")));
+ // assertTrue(info.needsToBeChopped(ke("x", "aa", "a")));
+ // assertTrue(info.needsToBeChopped(ke("x", null, null)));
+ // assertFalse(info.needsToBeChopped(ke("x", "c", "b")));
+ // assertFalse(info.needsToBeChopped(ke("y", "c", "b")));
+ // assertFalse(info.needsToBeChopped(ke("x", "c", "bb")));
+ // assertTrue(info.needsToBeChopped(ke("x", "b", "a")));
Review Comment:
Again, not sure which case is no longer valid that made these no longer be
needed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
