[GitHub] [pulsar] merlimat commented on a change in pull request #11490: [ManagedLedger] Compress managed ledger info

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

merlimat commented on a change in pull request #11490:
URL: https://github.com/apache/pulsar/pull/11490#discussion_r681416488



##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/MetaStoreImpl.java
##########
@@ -47,9 +54,31 @@
     private final MetadataStore store;
     private final OrderedExecutor executor;
 
+    public static final short MAGIC_MANAGED_LEDGER_INFO_METADATA = 0x0b9c;
+    private final CompressionType compressionType;
+
     public MetaStoreImpl(MetadataStore store, OrderedExecutor executor) {
         this.store = store;
         this.executor = executor;
+        this.compressionType = CompressionType.NONE;
+    }
+
+    public MetaStoreImpl(MetadataStore store, OrderedExecutor executor, String compressionType) {
+        this.store = store;
+        this.executor = executor;
+        CompressionType finalCompressionType;
+        if (compressionType != null) {
+            try {
+                finalCompressionType = CompressionType.valueOf(compressionType);
+            } catch (Exception e) {
+                log.warn("Failed to get compression type {}, disable managedLedgerInfo compression, error msg: {}.",

Review comment:
       If the compression type is not valid we should just throw exception instead of trying to handle and disable compression. This could lead to situation in which someone thinks compression is enabled but it really is failing.

##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/MetaStoreImpl.java
##########
@@ -47,9 +54,31 @@
     private final MetadataStore store;
     private final OrderedExecutor executor;
 
+    public static final short MAGIC_MANAGED_LEDGER_INFO_METADATA = 0x0b9c;

Review comment:
       We should try to use a magic number with the most significant bit set to 1 because that should be guaranteed to be a non-valid protobuf sequence

##########
File path: managed-ledger/src/main/proto/MLDataFormats.proto
##########
@@ -124,3 +124,8 @@ message ManagedCursorInfo {
     // Store which index in the batch message has been deleted
     repeated BatchedEntryDeletionIndexInfo batchedEntryDeletionIndexInfo = 7;
 }
+
+message ManagedLedgerInfoMetadata {
+    required string compressionType = 1;
+    required int32 unpressedSize = 2;

Review comment:
       ```suggestion
       required int32 uncompressedSize = 2;
   ```

##########
File path: managed-ledger/src/main/proto/MLDataFormats.proto
##########
@@ -124,3 +124,8 @@ message ManagedCursorInfo {
     // Store which index in the batch message has been deleted
     repeated BatchedEntryDeletionIndexInfo batchedEntryDeletionIndexInfo = 7;
 }
+
+message ManagedLedgerInfoMetadata {
+    required string compressionType = 1;

Review comment:
       I think we could just copy the same enum type here, or even declare it as `int` and use the other enum type 




-- 
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: commits-unsubscribe@pulsar.apache.org

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