Skip to content
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

session: add transaction start timestamp to the log when retrying #8091

Merged
merged 3 commits into from
Nov 7, 2018

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Oct 29, 2018

What problem does this PR solve?

The retry log does not contain the current retrying transaction timestamp and also which transaction it was retried.

What is changed and how it works?

Add a log of transaction start timestamp when retrying.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    The log will look like below:
2018/10/29 14:38:57.074 session.go:501: [warning] con:10 txn(403910200412078080) has retried txn(403910200370135040)
  • Need to cherry-pick to the release branch

PTAL @zimulala

@jackysp jackysp added the type/enhancement The issue or PR belongs to an enhancement. label Oct 29, 2018
@jackysp jackysp requested a review from zimulala October 29, 2018 07:20
@@ -497,6 +498,7 @@ func (s *session) retry(ctx context.Context, maxCnt uint) error {
}
s.StmtCommit()
}
log.Warnf("con:%d txn(%d) has retried txn(%d)", connID, s.GetSessionVars().TxnCtx.StartTS, orgStartTS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/txn(%d)/txn:%d?
I see other places use "txn:".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimulala the other place is txn, this is txn start ts.
@lilin90 need help to translate like "transaction A which transaction start timestamp/transaction ID is xxx has finished (may be not success e.g. meets some errors) and it retried the statements which are from transaction B which transaction start timestamp/transaction ID is xxx"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackysp I think it's easier to understand this way:

Transaction A, with the transaction start timestamp (transaction ID) xxx, has finished retrying (might fail, such as meeting some errors) all the statements in transaction B, with the transaction start timestamp (transaction ID) yyy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used the Unix naming method to keep it simple. PTAL @zimulala

@jackysp jackysp added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Nov 5, 2018
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jackysp
Copy link
Member Author

jackysp commented Nov 5, 2018

PTAL @zimulala

@@ -497,6 +498,8 @@ func (s *session) retry(ctx context.Context, maxCnt uint) error {
}
s.StmtCommit()
}
log.Warnf("con:%d retrying_txn_start_ts:%d original_txn_start_ts:(%d)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about combine retry_start_ts and origin_ts to L488 L490's log

Copy link
Member Author

@jackysp jackysp Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried it. It makes them so annoying because of so many same info in these lines if the transaction is large. We could use the connection ID to find the previous log,

@jackysp
Copy link
Member Author

jackysp commented Nov 6, 2018

PTAL @lysu

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/release-blocker This issue blocks a release. Please solve it ASAP. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants