[GitHub] [pulsar] aloyszhang commented on a change in pull request #8618: [PIP 70][Issue 8617] Introduce lightweight broker entry metadata

[GitBox <gi...@apache.org>]

aloyszhang commented on a change in pull request #8618:
URL: https://github.com/apache/pulsar/pull/8618#discussion_r540628357



##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -103,8 +107,13 @@ public void setCloseWhenDone(boolean closeWhenDone) {
 
     public void initiate() {
         if (STATE_UPDATER.compareAndSet(OpAddEntry.this, State.OPEN, State.INITIATED)) {
-            ByteBuf duplicateBuffer = data.retainedDuplicate();
 
+            ByteBuf duplicateBuffer = data.retainedDuplicate();
+            if (ml.getConfig().isBrokerEntryMetaEnabled()) {
+                duplicateBuffer = Commands.addBrokerEntryMetadata(duplicateBuffer,

Review comment:
       As we described in PIP-70, this feature can support continous sequenceId for Pulsar entry in the future, so, I think do these operations here is in favour of the future features.  For this broker-timestamp feature only,  move this logic as you suggested is a good choice. But for further extension,  maybe do this logic by ManagedLedger is better.  What's your opinion about this?
   
   




----------------------------------------------------------------
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.

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