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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
