Remove Managed Ledger metadata text format #394
Conversation
merlimat
force-pushed
the
remove-text-format
branch
from
f2a7717 to
8eaf21e
Compare
|
@rdhabalia @saandrews Please take a look |
There was a problem hiding this comment.
We should handle it even if parsing text fails.
There was a problem hiding this comment.
What do you mean? If we cannot parse the protobuf we need to throw exception..
There was a problem hiding this comment.
I didn't notice that the method is throwing exception. I was thinking that we are not handling the exception when it fails here.
|
The change looks good. We have to hold this pull request until we are ready with all the dependent tools to handle binary format. |
The tool to get the ML metadata in text format is already in 1.17: |
|
Well, we have some internal tools as well :) I will merge it as soon as we are ready. |
|
@saandrews any update on this one? Should we move to 1.19? |
|
@merlimat Yeah, lets move it to 1.19 |
|
@saandrews sure, but let's try to get this in soon. This is reducing the ZK metadata footprint considerably. Also this gives freedom to update the metadata format without worrying about backward compatibility issues. |
merlimat
added
the
type/enhancement
merlimat
force-pushed
the
remove-text-format
branch
from
8eaf21e to
ec7a7e2
Compare
|
@saandrews @rdhabalia Since #526 is already in, can we merge this one? |
|
@saandrews @rdhabalia Ping |
|
retest this please |
|
@saandrews @rdhabalia Any update on this? I'd like to add some new optional fields in the cursor protobuf data and it would be difficult to do it with the text format (for the schema migration and deployment rollback). |
|
Maybe we can merge and defer this in our branch until we are ready(though won't prefer branching). @joefk ? |
|
I am good with merging this. |
@joefk Still need "formal" PR approval, otherwise it cannot be merged 😃 |
saandrews
left a comment
saandrews
left a comment
There was a problem hiding this comment.
We are good to go now :)
|
🎉 |
* Support sendTimeout * Substitute NonBlockIfQueueFull for BlockIfQueueFull * Rename NonBlockIfQueueFull to DisableBlockIfQueueFull * Replaced createdAt with sentAt which added in PR 397. * Check and fail timeout messages before flushing batch * Optimize code * Remove redundant checking
### Motivation The decode method in `KafkaEntryFormatter` is time-consuming among which the process of constructing `MemoryRecordsBuilder` takes the highest proportion of time. ### Designing Therefore our aims are: 1. overwrites `BaseOffset` field in byte buffer directly to avoid time-consuming iteration which is mainly used for assigning offsets 2. concatenates multiple record batches into one `MemoryRecords` object to better simulate kafka's behavior ### Modifications - remove the iteration of continuously appending records in the `MemoryRecordsBuilder` - change to one-to-one correspondence between batch and entry
4 participants
Motivation
Complete the steps described in #281.
In v1.17, (#282) ManagedLedger started supporting both text and binary versions of protobuf encoding for the metadata in ZK.
In v1.18, we can leave only the option for binary format. If a rollback to 1.17 becomes necessary, it won't be a problem since 1.17 can deserialize the binary format.
Modifications
Make binary format the only option for managed ledger metadata.