[GitHub] [hadoop] goiri commented on a diff in pull request #5664: HDFS-17009. RBF: state store putAll should also return failed records

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

goiri commented on code in PR #5664:
URL: https://github.com/apache/hadoop/pull/5664#discussion_r1195729233


##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreFileBaseImpl.java:
##########
@@ -372,12 +373,12 @@ public boolean isDriverReady() {
   }
 
   @Override
-  public <T extends BaseRecord> boolean putAll(
+  public <T extends BaseRecord> StateStoreOperationResult putAll(
       List<T> records, boolean allowUpdate, boolean errorIfExists)
           throws StateStoreUnavailableException {
     verifyDriverReady();
     if (records.isEmpty()) {
-      return true;
+      return new StateStoreOperationResult(Collections.emptyList(), true);

Review Comment:
   Can you have a constructor empty which sets to true and empty?



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreFileBaseImpl.java:
##########
@@ -402,7 +403,7 @@ public <T extends BaseRecord> boolean putAll(
           if (metrics != null) {
             metrics.addFailure(monotonicNow() - start);
           }
-          return false;
+          return new StateStoreOperationResult(Collections.singletonList(primaryKey), false);

Review Comment:
   Why not adding the constructor with a primary key to StateStoreOperationResult?



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreMySQLImpl.java:
##########
@@ -161,10 +162,10 @@ public <T extends BaseRecord> QueryResult<T> get(Class<T> clazz)
   }
 
   @Override
-  public <T extends BaseRecord> boolean putAll(
+  public <T extends BaseRecord> StateStoreOperationResult putAll(
       List<T> records, boolean allowUpdate, boolean errorIfExists) throws IOException {
     if (records.isEmpty()) {
-      return true;
+      return new StateStoreOperationResult(Collections.emptyList(), true);

Review Comment:
   Same as before.
   We can even have:
   ```
   StateStoreOperationResult.empty()
   ```



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreFileBaseImpl.java:
##########
@@ -479,11 +483,13 @@ private <T extends BaseRecord> Void writeRecordToFile(AtomicBoolean success,
     } catch (IOException e) {
       LOG.error("Cannot write {}", recordPathTemp, e);
       recordWrittenSuccessfully = false;
+      failedRecordsList.add(getPrimaryKey(entry.getValue()));

Review Comment:
   Or even the primary key.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreFileBaseImpl.java:
##########
@@ -479,11 +483,13 @@ private <T extends BaseRecord> Void writeRecordToFile(AtomicBoolean success,
     } catch (IOException e) {
       LOG.error("Cannot write {}", recordPathTemp, e);
       recordWrittenSuccessfully = false;
+      failedRecordsList.add(getPrimaryKey(entry.getValue()));

Review Comment:
   Extract getValue()?



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

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


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