Conversation
blp
added
performance
connectors
mythical-fred
left a comment
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. Clean refactor: moving the transaction sender into TransactionInfo is the right ownership model, and the AdvanceTransaction enum makes advance_transaction_state() easier to follow. The new transaction_msecs and transaction_records fields on the API and the two histograms (run time + commit time) are genuinely useful for debugging slow commits. Docs updated.
| }); | ||
| self.circuit | ||
| .start_transaction() | ||
| .expect("should have been able to start transaction"); |
There was a problem hiding this comment.
are these now panics?
They didn't seem to be previously.
There was a problem hiding this comment.
They are now panics. I reviewed the code that they call and it seemed to me that any failure is a bug in the caller.
| pub fn tid(&self) -> Option<TransactionId> { | ||
| match self { | ||
| TransactionState::None => None, | ||
| TransactionState::Started { tid, .. } | TransactionState::Committing { tid, .. } => { |
There was a problem hiding this comment.
I didn't know you can do this
There was a problem hiding this comment.
Yep! It is occasionally a useful feature of matches.
| @@ -4834,8 +4918,8 @@ impl TransactionInitiators { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
How come no one complained about these two being in the wrong order?
There was a problem hiding this comment.
I don't think there's a preferred order.
I didn't intentionally reorder these; it was a mistake as a result of intermediate changes that I ended up reverting before submission.
| transaction_state: TransactionState, | ||
|
|
||
| /// For sending updates to the coordination status to the coordinator. | ||
| sender: tokio::sync::watch::Sender<TransactionCoordination>, |
There was a problem hiding this comment.
Do we also add the sender in the stats struct in other places?
There was a problem hiding this comment.
I don't understand this question.
This isn't a stats struct; it's part of what we use to track transactions.
There was a problem hiding this comment.
Based on the name I assumed it's for stats
There was a problem hiding this comment.
Maybe I should change the name. Which name is the one that causes the misconception?
There was a problem hiding this comment.
"Info". But I guess it's not "Stats"
There was a problem hiding this comment.
I agree that TransactionInfo isn't the best name. I think @ryzhyk invented it. Maybe TransactionManager or TransactionTracker or even just Transaction.
| } | ||
| (TransactionState::None | TransactionState::Started { .. }, true) => Some(true), | ||
| (TransactionState::Committing { .. }, true) => { | ||
| error!( |
There was a problem hiding this comment.
Is this expected to be unreachable?
What will happen if this is reached? Will the pipeline ever recover from this state?
There was a problem hiding this comment.
I think that this is unreachable code. This code is moved from elsewhere in the same file, where it used error!. I think that we could change it to unreachable!() (but @ryzhyk might know better).
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
ryzhyk
left a comment
ryzhyk
left a comment
There was a problem hiding this comment.
Glad you got rid of the duplicate fields in the global metrics.
crates/adapters/src/controller.rs
Outdated
| commit_progress.remaining | ||
| ); | ||
| metrics.histogram( | ||
| "transaction_run_seconds", |
There was a problem hiding this comment.
This name may be too easy to misinterpret (although the description explains what it means). Maybe we should rename it to something more verbose (but possibly awkward): transaction_pre_commit_time_seconds
There was a problem hiding this comment.
What do you think of calling this the ingest phase? transaction_ingest_time_seconds
crates/adapters/src/controller.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn start(&self) -> Option<Instant> { |
There was a problem hiding this comment.
Maybe rename to start_time? Otherwise the name suggests that the method changes transaction state.
| /// | ||
| /// - [TransactionStatus::CommitInProgress]: Time that this transaction has | ||
| /// been committing. | ||
| pub transaction_msecs: Option<u64>, |
There was a problem hiding this comment.
These new fields will need to be added to the Python SDK.
blp
force-pushed
the
txn-stats
branch
5 times, most recently
from
a156f66 to
f55e05a
Compare
Tested manually. Signed-off-by: Ben Pfaff <blp@feldera.com>
4 participants
Tested manually.