[GitHub] [ozone] jojochuang commented on a diff in pull request #5167: HDDS-9146. Potential data loss with HSync due to deletedTable entry having the same block as keyTable entry's

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

jojochuang commented on code in PR #5167:
URL: https://github.com/apache/ozone/pull/5167#discussion_r1289540502


##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyLocationInfoGroup.java:
##########
@@ -177,15 +184,11 @@ void addAll(long versionToAdd, List<OmKeyLocationInfo> locationInfoList) {
 
   @Override
   public String toString() {
-    StringBuilder sb = new StringBuilder();
-    sb.append("version:").append(version).append(" ");
-    sb.append("isMultipartKey:").append(isMultipartKey).append(" ");
-    for (List<OmKeyLocationInfo> kliList : locationVersionMap.values()) {
-      for (OmKeyLocationInfo kli: kliList) {
-        sb.append(kli.getLocalID()).append(" || ");
-      }
-    }
-    return sb.toString();
+    return "OmKeyLocationInfoGroup{" +

Review Comment:
   why the change? StringBuilder looks more elegant and performant.



##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestHSync.java:
##########
@@ -131,6 +146,47 @@ public static void teardown() {
     }
   }
 
+  @Test
+  public void testKeyHSyncThenClose() throws IOException {
+    // Check that deletedTable should not have keys with the same block as in
+    // keyTable's when a key is hsync()'ed then close()'d.
+
+    // Set the fs.defaultFS
+    final String rootPath = String.format("%s://%s/",
+        OZONE_OFS_URI_SCHEME, CONF.get(OZONE_OM_ADDRESS_KEY));
+    CONF.set(CommonConfigurationKeysPublic.FS_DEFAULT_NAME_KEY, rootPath);
+
+    final String dir = OZONE_ROOT + bucket.getVolumeName()
+        + OZONE_URI_DELIMITER + bucket.getName();
+
+    String data = "random data";
+    final Path file = new Path(dir, "file-hsync-then-close");
+    try (FileSystem fs = FileSystem.get(CONF)) {
+      try (FSDataOutputStream outputStream = fs.create(file, true)) {
+        outputStream.write(data.getBytes(UTF_8), 0, data.length());
+        outputStream.hsync();
+      }
+    }
+
+    OMMetadataManager ommm = cluster.getOzoneManager().getMetadataManager();
+    // deletedTable should not have an entry for file at all in this case
+    try (TableIterator<String,
+        ? extends Table.KeyValue<String, RepeatedOmKeyInfo>>
+        tableIter = ommm.getDeletedTable().iterator()) {
+      while (tableIter.hasNext()) {
+        Table.KeyValue<String, RepeatedOmKeyInfo> kv = tableIter.next();
+        String key = kv.getKey();
+        if (key.startsWith(file.toString())) {

Review Comment:
   shouldn't it fail if deletedTable is not empty? Also key string will not match file string anyway.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java:
##########
@@ -301,6 +323,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
               exception, getOmRequest().getUserInfo()));
       processResult(commitKeyRequest, volumeName, bucketName, keyName,
           omMetrics, exception, omKeyInfo, result);
+    } else {
+      // Debug logging for HSync
+      if (LOG.isDebugEnabled()) {

Review Comment:
   this check is redundant.



-- 
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@ozone.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org