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

Less aborted txns #1551

Merged
merged 2 commits into from
Jul 12, 2019
Merged

Conversation

patrick-east
Copy link
Contributor

This changes a few things transaction related..

  • Update the Rego prepared query objects to no longer re-use old storage transactions from the original Rego object. The txn must be specified on each Eval() if you want to use a specific one, otherwise a new one will be opened and closed automatically
  • Pass the transaction through to the decision log mask eval

Check out the individual commit messages for some more details.

@patrick-east patrick-east requested a review from tsandall July 9, 2019 21:59
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

The changes look good. See my note on the test. Once that's resolved we can merge.

rego/rego_test.go Outdated Show resolved Hide resolved
rego/rego_test.go Outdated Show resolved Hide resolved
rego/rego_test.go Outdated Show resolved Hide resolved
We were only using it on the prepare step, but we needed to use the
passed in txn each time we eval the prepared query.

Signed-off-by: Patrick East <east.patrick@gmail.com>
tsandall
tsandall previously approved these changes Jul 11, 2019
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

Changes look good. See note about moving the mock. Once that's done LGTM.

storage/mock/mock.go Outdated Show resolved Hide resolved
When doing a prepared query Eval() or Partial() we would build an
EvalContext that defaulted to using the original Rego objects
transaction. This is problematic as that one might have been aborted,
or just be old. The expectation if you don't specify one at Eval()
time was that you would get a new one at the current state of the
store, so we should keep the prepared Eval() to work the same way.

This means that if you did have a specific transaction you wanted
to use you would now need to pass it in instead of only setting
it on the original Rego object. This changed some tests but in real
usage it shouldn't affect much.

This also fixes an issue where we would abort transactions auto
created on the EvalContext way too early. The helper (internal to
the Rego package) now returns a finish function that will do the
right thing and abort it at an appropriate time.

Signed-off-by: Patrick East <east.patrick@gmail.com>
Copy link
Member

@tsandall tsandall 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants