[GitHub] [pulsar] codelipenghui commented on issue #16763: PIP-195: New bucket based delayed message tracker

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

codelipenghui commented on issue #16763:
URL: https://github.com/apache/pulsar/issues/16763#issuecomment-1219606491

   > Delete message index bucket
   After merging buckets, the delayed message tracker will delete the old bucket.
   Also, when all the delayed messages of all snapshots in the bucket are scheduled then tracker will delete that bucket.
   
   We need to explain how snapshot can be guaranteed to be deleted without being missed.
   
   > Admin-API changes for clear snapshot
   
   Could you please add more context about why we need this API? to rebuild the index?
   It should be a risk if only clear the index without any cursor rewind or index rebuilding.
   The consumer will not be able to receive delayed messages, no?
   
   For the metrics we are using `pulsar_delayed_message_index_*` https://github.com/apache/pulsar/pull/15867/files#diff-b3f64ff76fdabb9e471435147298c3c707cbecae09023f0d86cf06f6ad78cbdaR352. It's better to keep the metrics name
   consistent.
   
   ```
   pulsar_delayed_message_index_bucket_total | Gauge
   pulsar_delayed_message_index_snapshot_size_bytes | Gauge
   pulsar_delayed_message_index_bucket_op_latency_ms {type="load/merge/del"} | Histogram
   pulsar_delayed_message_index_bucket_op_failed_count | Counter
   
   ```
   
   For the BucketSnapshotFormat.proto
   
   Is it better to split SnapshotMetadata and SnapshotData into 2 commands? It's easier to read. The metadata message have bitset and scheduleTime, the data message only has the sorted list. And we'd better change `BucketSnapshotFormat.proto` to `DelayedMessageIndexBucketSnapshotFormat.proto`
   
   >     CompletableFuture<Void> saveBucketAllSnapshots(long bucketId, List<BucketSnapshotFormat.BucketSnapshot> bucketSnapshots);
   
   It's a little confusing for this one. For each bucket, we should only have one snapshot, for each entry in the snapshot, we should use a different name, snapshot record or something.
   
   > CompletableFuture<Long> createBucket();
   
   I think this one is used to create a snapshot?
   
   > CompletableFuture<Void> deleteBucket(long bucketId);
   
   is this one used to delete a snapshot?
   
   >     void start() throws Exception;
   
   What does this method really do? Does it look like `initialize()`?
   
   
   It is hard to comment on the issue, it looks like we need to find a new approach to create and review the proposal. 
   


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