ChenSammi commented on code in PR #9364:
URL: https://github.com/apache/ozone/pull/9364#discussion_r2904031603
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java:
##########
@@ -409,6 +411,46 @@ private String addToBatch(Queue<Entry> buffer,
BatchOperation batchOperation) {
try {
addToBatchWithTrace(omResponse,
() -> response.checkAndUpdateDB(omMetadataManager,
batchOperation));
+
+ // This is a strawman approach and requires some discussion
+ // with the community on approach.
+ //
+ // At the moment any response type which we want to capture a
+ // OmCompletedRequestInfo for is required to implement the
+ // interface HasCompletedRequestInfo and the method
+ // getCompletedRequestInfo() and we then have this extra step
+ // here in the double buffer to capture the rows.
+ //
+ // It would seem ideal that the double buffer shouldn't have to
+ // know/care that there is the concept of capturing this
+ // OmCompletedRequestInfo row for certain response times but the
+ // approach described above seemed like the least invasive
+ // approach overall. I am open to other views.
+ //
+ // Other approaches I considered:
+ // - adding a similar getCompletedRequestInfo method to each
+ // *request* type which want to capture a row for. The downside
+ // of this was that since requests are not part of the
+ // addToBatch flow the OmCompletedRequestInfo instance then had
+ // to be passed through from the request to the relevant
+ // response constructors and this created quite a bit of code
+ // churn which I perceived as messy
+ //
+ // * in terms of the actual data capture, rather than this
+ // "instanceof" approach in this class I toyed with
+ // having each response type which we want to capture a row for
+ // capturing it itself in its on implementation of addDBBatch
+ // (i.e. no need for any new code in this class). This
+ // seemed to be a bit messier to me in terms of code duplication
+ //
+ if (response instanceof HasCompletedRequestInfo) {
Review Comment:
@gardenia , I think you can just add the following in each involved
request's OMClientResponse#addToDBBatch(), addToDBBatch() is well DB is updated
for each request, and OzoneManagerDoubleBuffer is reponsible to flush the batch
to DB, so add the CompletedRequestInfoTable update in addToDBBatch should be
fine.
```
omMetadataManager.getCompletedRequestInfoTable().putWithBatch(
batchOperation, completedRequestInfo.getTrxLogIndex(),
completedRequestInfo);
```
For example, OMFileCreateResponse,
```
public void addToDBBatch(OMMetadataManager omMetadataManager,
BatchOperation batchOperation) throws IOException
{
super.addToDBBatch(omMetadataManager, batchOperation);
omMetadataManager.getCompletedRequestInfoTable().putWithBatch(
batchOperation, getTrxLogIndex(), getCompletedRequestInfoArgs());
}
```
updateID in each object is the TrxLogIndex.
--
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]
