gardenia commented on code in PR #9364:
URL: https://github.com/apache/ozone/pull/9364#discussion_r2742499110
##########
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:
@ChenSammi / @errose28 I would be grateful for your feedback on this
approach to capturing the data for the new entity class by adding it to the
transaction batch here OzoneManagerDoubleBuffer.
I discussed this briefly with @errose28 on the last community call and have
made some revisions informed by that discussion although it is not exactly what
we discussed. However, I the comment above explains my thought process (i.e.
that it could have been added to the addToDBBatch method of each relevant
response implementation rather than have any code footprint in
OzoneManagerDoubleBuffer but that approach was messier than I would have
liked). I'd be happy to hear alternate takes
NOTE: I'm aware this is lacking unit tests but I'd like to come to some
consensus on where the code is going to live first
--
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]
