HDDS-14004. EventNotification: capture data in the CompletedRequestInfo ledger#9364
HDDS-14004. EventNotification: capture data in the CompletedRequestInfo ledger#9364gardenia wants to merge 1 commit intoapache:HDDS-13513_Event_Notification_FeatureBranchfrom
Conversation
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
swamirishi
left a comment
swamirishi
left a comment
There was a problem hiding this comment.
thanks for the patch @gardenia have left some comments inline
| // request info "ledger" table is consistent with the processed | ||
| // raits events? e.g. OzoneManagerDoubleBuffer? | ||
|
|
||
| try (BatchOperation batchOperation = omMetadataManager.getStore() |
There was a problem hiding this comment.
We don't need a rocksdb batch. Flush should happen as part of double buffer flush
Look at this change https://github.com/apache/ozone/pull/8779/files#r2641773095
There was a problem hiding this comment.
@swamirishi I have implemented a rough approach for plumbing this into double buffer flush as you suggested but I am not happy with the way I have plumbed it in and feel it needs some community feedback.
I would be happy to hear your feedback or can discuss at the next community call.
As I commented inline, I wonder if it is better to add the populating of the ledger rows into the response implementation of each request type we want to capture (e.g. OMVolumeCreateResponse::addToBatch, OMKeyCommitResponse::addToBatch, etc) which would then dispense with the need for the class I have at the moment which is a single place where the ledger rows are created.
cc: @errose28
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
gardenia
force-pushed
the
HDDS-14004
branch
2 times, most recently
from
c9ca8f7 to
60a1b91
Compare
| // (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) { |
There was a problem hiding this 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
gardenia
force-pushed
the
HDDS-14004
branch
4 times, most recently
from
590139e to
d6e3906
Compare
|
Hi @gardenia , is this the second patch, and ready for review? |
@ChenSammi Yes. Thank you. The unit testing is not comprehensive but I would appreciate some feedback on the general approach (in particular of having the HasCompletedRequestInfo interface which response types we want to capture have to implement) |
5 participants
Please describe your PR in detail:
In OzoneManagerStateMachine when certain write requests complete successfully a summary of that request will be written to a new rocksdb "ledger" table named CompletedRequestInfo.
The intent is that a consumer can use that data to generate event notifications about changes on the filesystem.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14004
How was this patch tested?
unit tests