-
Notifications
You must be signed in to change notification settings - Fork 535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Documentation in OpLifecycle #23636
base: main
Are you sure you want to change the base?
Conversation
Thanks for doing this! Overall suggestion - Can you move the "Grouped batching" section above the "Compression" section? Then your new part about grouping being required will have enough context to make more sense. |
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
Compressing a batch yields a batch with the same number of messages. It compresses all the content, shifting the compressed payload into the first op, | ||
leaving the rest of the batch's messages as empty placeholders to reserve sequence numbers for the compressed messages. | ||
|
||
### Single message batch update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd update this to emphasize the current reality more. So before this paragraph, start ### Only single-message batches are compressed
. Then under that heading you can explain how compression can only be enabled if Grouped Batching is enabled, and that it wasn't always this way, and explain about how compressing a multi-message batch would yield empty placeholder ops (btw we don't say "operations"), but we don't do that anymore (but we do support reading those ops still)
## How it works (Grouped Batching disabled) -Outdated- | ||
|
||
### Outdated - Main code no longer creates empty ops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## How it works (Grouped Batching disabled) -Outdated- | |
### Outdated - Main code no longer creates empty ops | |
## Legacy behavior - Compression without Grouped Batching | |
### IMPORTANT - As of 2.20.0, we no longer compress ungrouped batches, but we do need to read such ops - read on to learn what these legacy ops look like |
Or something like that
Description
When the changes in the feature are merged, the current README.md of OpLifecycle will be outdated, therefore we need to update it to match the current flow of ops.
Execution Plan
Update the README.md of the OpLifecycle folder.
Acceptance Criteria
The new README.md reflects the new flow of OpLifecycle.
Reviewer Guidance
Let me know if there's a better way to write the changes in the docs
Fixes: AB#28650