[GitHub] [hbase] Apache9 commented on a diff in pull request #5047: HBASE-27649 WALPlayer does not properly dedupe overridden cell versions

["Apache9 (via GitHub)" <gi...@apache.org>]

Apache9 commented on code in PR #5047:
URL: https://github.com/apache/hbase/pull/5047#discussion_r1117928502


##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java:
##########
@@ -131,6 +133,84 @@ public void testPlayingRecoveredEdit() throws Exception {
     assertTrue(TEST_UTIL.countRows(tn) > 0);
   }
 
+  /**
+   * Tests that when you write multiple cells with the same timestamp they are properly sorted by
+   * their sequenceId in WALPlayer/CellSortReducer so that the correct one wins when querying from
+   * the resulting bulkloaded HFiles. See HBASE-27649
+   */
+  @Test
+  public void testWALPlayerBulkLoadWithOverriddenTimestamps() throws Exception {
+    final TableName tableName = TableName.valueOf(name.getMethodName() + "1");
+    final byte[] family = Bytes.toBytes("family");
+    final byte[] column1 = Bytes.toBytes("c1");
+    final byte[] column2 = Bytes.toBytes("c2");
+    final byte[] row = Bytes.toBytes("row");
+    Table table = TEST_UTIL.createTable(tableName, family);
+
+    long now = System.currentTimeMillis();
+    // put a row into the first table
+    Put p = new Put(row);
+    p.addColumn(family, column1, now, column1);
+    p.addColumn(family, column2, now, column2);
+
+    table.put(p);
+
+    byte[] lastVal = null;
+
+    for (int i = 0; i < 50; i++) {
+      lastVal = Bytes.toBytes(ThreadLocalRandom.current().nextLong());
+      p = new Put(row);
+      p.addColumn(family, column1, now, lastVal);
+
+      table.put(p);
+
+      // wal rolling is necessary to trigger the bug. otherwise no sorting
+      // needs to occur in the reducer because it's all sorted and coming from a single file.
+      if (i % 10 == 0) {
+        WAL log = cluster.getRegionServer(0).getWAL(null);
+        log.rollWriter();
+      }
+    }
+
+    WAL log = cluster.getRegionServer(0).getWAL(null);
+    log.rollWriter();
+    String walInputDir = new Path(cluster.getMaster().getMasterFileSystem().getWALRootDir(),
+      HConstants.HREGION_LOGDIR_NAME).toString();
+
+    Configuration configuration = new Configuration(TEST_UTIL.getConfiguration());
+    String outPath = "/tmp/" + name.getMethodName();
+    configuration.set(WALPlayer.BULK_OUTPUT_CONF_KEY, outPath);
+    configuration.setBoolean(WALPlayer.MULTI_TABLES_SUPPORT, true);
+
+    WALPlayer player = new WALPlayer(configuration);
+    assertEquals(0, ToolRunner.run(configuration, player,
+      new String[] { walInputDir, tableName.getNameAsString() }));
+
+    Get g = new Get(row);
+    Result result = table.get(g);
+    byte[] value = CellUtil.cloneValue(result.getColumnLatestCell(family, column1));
+    assertTrue(
+      "Expected " + Bytes.toStringBinary(column1) + " latest value to be equal to lastVal="
+        + Bytes.toStringBinary(lastVal) + " but was " + Bytes.toStringBinary(value),
+      Bytes.equals(lastVal, value));
+
+    table = TEST_UTIL.truncateTable(tableName);
+    g = new Get(row);
+    result = table.get(g);
+    assertTrue("expected row to be empty after truncate but got " + result, result.isEmpty());

Review Comment:
   Use assertThat and Matchers.empty?



##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALPlayer.java:
##########
@@ -131,6 +133,84 @@ public void testPlayingRecoveredEdit() throws Exception {
     assertTrue(TEST_UTIL.countRows(tn) > 0);
   }
 
+  /**
+   * Tests that when you write multiple cells with the same timestamp they are properly sorted by
+   * their sequenceId in WALPlayer/CellSortReducer so that the correct one wins when querying from
+   * the resulting bulkloaded HFiles. See HBASE-27649
+   */
+  @Test
+  public void testWALPlayerBulkLoadWithOverriddenTimestamps() throws Exception {
+    final TableName tableName = TableName.valueOf(name.getMethodName() + "1");
+    final byte[] family = Bytes.toBytes("family");
+    final byte[] column1 = Bytes.toBytes("c1");
+    final byte[] column2 = Bytes.toBytes("c2");
+    final byte[] row = Bytes.toBytes("row");
+    Table table = TEST_UTIL.createTable(tableName, family);
+
+    long now = System.currentTimeMillis();

Review Comment:
   Use EnvironmentEdgeManager.currentTime if possible



-- 
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: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org