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

workload/ycsb: Use StmtContext instead of Stmt #39525

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

jeffrey-xiao
Copy link
Contributor

@jeffrey-xiao jeffrey-xiao commented Aug 10, 2019

StmtContext allows context.DeadlineExceeded error to be returned consistently. Using Stmt would sometimes return sql: transaction has already been committed or rolled back causing the workload to fail.

Additionally, sometimes a context cancellation during a transaction can result in sql.ErrNoRows instead of the appropriat econtext.DeadlineExceeded. In this case, we just return ctx.Err().

See lib/pq#874.

Fixes #39521.

workload run ycsb --drop --insert-count=1000000 --splits=100 --workload=F --concurrency=64 --ramp=1m --duration=10m terminates successfully with this change.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm: The reason for the deadline was the --duration flag?

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

Copy link
Contributor Author

@jeffrey-xiao jeffrey-xiao left a comment

Choose a reason for hiding this comment

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

context.DeadlineExceeded comes from the --ramp flag which gradually starts all workers with a rampCtx then restarts them with the main context.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

context.DeadlineExceeded comes from the --ramp flag which gradually starts all workers with a rampCtx then restarts them with the main context.

👌

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

return err
})
if err == gosql.ErrNoRows {
Copy link
Member

Choose a reason for hiding this comment

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

if err == gosql.ErrNoRows && ctx.Err() != nil {

Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

LGTM once that comment is addressed.

StmtContext allows context.DeadlineExceeded error to be returned
consistently. Using Stmt would sometimes return "sql: transaction has
already been committed or rolled back" causing the workload to fail.

Additionally, sometimes a context cancellation during a transaction can
result in sql.ErrNoRows instead of the appropriate
context.DeadlineExceeded. In this case, we just return ctx.Err().

Release note: None
@jeffrey-xiao
Copy link
Contributor Author

TFTRs!

bors r+

craig bot pushed a commit that referenced this pull request Aug 14, 2019
39525: workload/ycsb: Use StmtContext instead of Stmt r=jeffrey-xiao a=jeffrey-xiao

`StmtContext` allows `context.DeadlineExceeded` error to be returned consistently. Using `Stmt` would sometimes return `sql: transaction has already been committed or rolled back` causing the workload to fail.

Additionally, sometimes a context cancellation during a transaction can result in sql.ErrNoRows instead of the appropriat econtext.DeadlineExceeded. In this case, we just return ctx.Err().

See lib/pq#874.

Fixes #39521. 

`workload run ycsb --drop --insert-count=1000000 --splits=100 --workload=F --concurrency=64 --ramp=1m --duration=10m` terminates successfully with this change.

Release note: None

Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
@craig
Copy link
Contributor

craig bot commented Aug 14, 2019

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: ycsb/F/nodes=3/cpu=32 failed
4 participants