GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Clarify the comments on retried transaction commits by blp · Pull Request #7822 · feldera/feldera

Conversation

blp commented Nov 3, 2026

Copy link

Copy Markdown

Member

Comments only, in crates/connectors/src/controller.rs. Review of feldera/cloud#2135 found them unclear, and that PR copied them from #7687.

Comment	Change
`TransactionInfo::last_api_commit`	"This is not any transaction up to `last_transaction_id`" now says that a request that names another earlier transaction fails, and why.
5 tests in `commit_transaction_id_tests`	Each comment starts with "Checks that", so it does not read like a description of a function's result.

No code changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          [connectors] Clarify the comments on retried transaction commits.

ce2c5c7

The comment on `last_api_commit` said "This is not any transaction up to
`last_transaction_id`", which did not say what it meant.  The test
comments read like descriptions of a function's result.  Now each test
comment starts with "Checks that".  Review of feldera/cloud#2135 found
the same text there.

Signed-off-by: Ben Pfaff <[email protected]>

blp requested a review from mihaibudiu

November 3, 2026 23:13

blp self-assigned this

Nov 3, 2026

blp added QA

Testing and quality assurance

rust Pull requests that update Rust code labels

Nov 3, 2026

mihaibudiu approved these changes

Nov 3, 2026

View reviewed changes

mythical-fred-oss Bot reviewed

Nov 3, 2026

View reviewed changes

Comment thread

crates/connectors/src/controller.rs

Comment on lines +7476 to 7479

		/// retry of that request succeeds without doing anything. A request that
		/// names another transaction up to `last_transaction_id` fails, because
		/// that transaction can be an automatic or connector transaction, or one
		/// that a restart lost.

mythical-fred-oss Bot Nov 3, 2026

Copy link

Copy Markdown

Contributor

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.

pitfall (medium): This doesn't match start_commit_transaction_from_api. The open API transaction is also "another transaction up to last_transaction_id", and a commit that names it succeeds. The stated reason is also incomplete: an older transaction that the API itself committed also fails, because only the most recent commit is remembered. The PR description says "another earlier transaction", but the comment doesn't. Suggested wording:

Suggested change

	/// retry of that request succeeds without doing anything. A request that
	/// names another transaction up to `last_transaction_id` fails, because
	/// that transaction can be an automatic or connector transaction, or one
	/// that a restart lost.
	/// retry of that request succeeds without doing anything. A request that
	/// names any other transaction, except the one that the API has open,
	/// fails. We can't accept every ID up to `last_transaction_id`, because
	/// that ID can belong to an automatic or connector transaction, or to one
	/// that a restart lost.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

blp Nov 4, 2026

Copy link

Copy Markdown

Member Author

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.

Fixed in 2e17784. I used your wording and added that an older API commit also fails, because only the last one is kept.

(Reply from Claude, on behalf of @blp.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mythical-fred-oss Bot commented Nov 3, 2026

Copy link

Copy Markdown

Contributor

Only comments change here, so no gates apply: behaviour, tests, docs and dependencies are untouched.

Ran	Result
`cargo test --locked -p dbsp_connectors --lib commit_transaction_id_tests`	5/5 pass
`pre-commit run --files crates/connectors/src/controller.rs`	pass

pitfall (medium): the new last_api_commit comment says something the code doesn't do (see the inline comment). Otherwise LGTM, and the "Checks that…" test comments read well.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          [connectors] Correct the comment on last_api_commit.

2e17784

The comment said that a commit of any other transaction up to
`last_transaction_id` fails, but a commit of the open API transaction
succeeds.  It also did not say that a retry of an older API commit
fails.  Found in review.

Signed-off-by: Ben Pfaff <[email protected]>

blp added this pull request to the merge queue

Nov 4, 2026

Merged via the queue into main with commit 79f0d30

Nov 4, 2026

1 check passed

Uh oh!

There was an error while loading. Please reload this page.

blp deleted the commit-transaction-docs branch

November 4, 2026 21:21

Reviewers

mythical-fred-oss[bot]

mythical-fred-oss[bot] left review comments

mihaibudiu

mihaibudiu approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

blp

Labels

Testing and quality assurance

rust

Pull requests that update Rust code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.