[rest] Do not replay a POST the server cannot absorb twice - #9122
Open
sundapeng wants to merge 3 commits into
Open
[rest] Do not replay a POST the server cannot absorb twice#9122sundapeng wants to merge 3 commits into
sundapeng wants to merge 3 commits into
Conversation
PartitionStatistics said only that its fields "may be negative, indicating that some data has been removed". That covers one of the two planes the class is read on, and consumers have been getting the other one wrong. On the delta plane — what a commit changed — a negative value is a decrement the server adds to what it holds. That is the existing meaning and nothing here changes it. On the observation plane — what listPartitions returns for a partition as it stands — a negative value means nobody ever reported that field, and 0 means an exact zero. Conflating them is not cosmetic: a consumer that reads unknown as zero plans against an empty partition that may hold a billion rows, and one that does arithmetic on it gets a number that is wrong rather than missing. So the plane is named in the javadoc, unknown gets a name (UNKNOWN, with isKnown() to test it rather than each caller comparing against -1), and unknown is documented as per field: a reporter that only knows the file count leaves the record count unknown and fills the rest. The fields stay primitive. Boxing them to express unknown as null would be a breaking change to a @public class, and the encoding above needs no new type. FileSystemSplitEnumerator now says PartitionStatistics.UNKNOWN where it said -1. Discovering partitions by listing directories measures nothing about what is inside them, which is what unknown already meant there; this is the same value under its own name.
…y counted FormatTableRollingFileWriter counts every row it writes and FormatTableSingleFileWriter knows the byte length of the file it closed. Both numbers are then dropped: closeAndGetCommitters returns only the committers, and prepareCommit wraps each one in a TwoPhaseCommitMessage that carries nothing else. Anything downstream that wants to know what a commit wrote has to go back to the filesystem and list it. This keeps the two numbers attached to the file they describe, in a new FormatTableWrittenFile that pairs the committer with them, and lets TwoPhaseCommitMessage carry it. Nothing reads them yet. TwoPhaseOutputStream.Committer is untouched. RenamingTwoPhaseOutputStream is @public, so adding a method to the type its committer() returns would break external implementations; the counts ride the paimon-core commit message instead.
The REST client retries a 429 or a 503 on any request, POST included. That is right for nearly everything it sends: registering a partition, creating a database, committing a snapshot the server already holds all land on the same state the second time, and the retry is the only defence against a rate limiter or a restarting node. It is wrong for a request that reports an increment. A 429 or a 503 can reach the client from an intermediary after the server already applied the request, so replaying it applies it again — and an increment applied twice is a wrong number that no caller can see. Nothing in the response distinguishes the two cases, which is why this has to be decided by the request rather than by the status. RESTRequest gains isRetrySafe(), defaulting to true so every existing request keeps the retry it has today. A request answering false is sent exactly once and the failure reaches the caller, which knows whether re-sending is safe. The mark travels in the HttpClientContext rather than in the request, so it never reaches the wire and survives whatever the exec chain does to the request object; @JsonIgnore keeps the getter out of the serialized body as well, and a test pins both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The REST client retries a 429 or a 503 on any request, POST included. That is right for nearly
everything it sends: registering a partition, creating a database, committing a snapshot the server
already holds all land on the same state the second time, and the retry is the only defence against a
rate limiter or a restarting node.
It is wrong for a request that reports an increment. A 429 or a 503 can reach the client from an
intermediary after the server already applied the request, so replaying it applies it again — and an
increment applied twice is a wrong number that no caller can see. Nothing in the response
distinguishes the two cases, which is why this has to be decided by the request rather than by the
status.
RESTRequestgainsisRetrySafe(), defaulting totrueso every existing request keeps the retryit has today. A request answering
falseis sent exactly once and the failure reaches the caller,which knows whether re-sending is safe.
Two details a reviewer will ask about
HttpClientContextrather than in the request, so it never reaches the wireand survives whatever the exec chain does to the request object.
isRetrySafe()is a getter on a serialized type, so it is@JsonIgnore; a test pins that it staysout of the body.
Compatibility
This PR adds no request that answers
false. The first one arrives with the partition-statisticswork for catalog-managed format tables, and the ordering matters: with that in and this out, an
automatic retry double-counts an increment and the server cannot tell a redelivery from a second
genuine increment.
API and Format
RESTRequest.isRetrySafe()is a newdefaultmethod returningtrue; existing implementations needno change. No format change.
Documentation
The contract is in the method javadoc.