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

db.go: return t.Rollback directly in the end of View function #41

Merged
merged 1 commit into from
Sep 12, 2017

Conversation

lorneli
Copy link
Contributor

@lorneli lorneli commented Sep 10, 2017

Calling t.Rollback is a little bit misleading in the end of View function, since this tx has succeeded. What t.Rollback actually does here is closing this tx. So close tx directly.

@codecov-io
Copy link

codecov-io commented Sep 10, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@4d3ab93). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #41   +/-   ##
=========================================
  Coverage          ?   84.91%           
=========================================
  Files             ?        9           
  Lines             ?     1849           
  Branches          ?        0           
=========================================
  Hits              ?     1570           
  Misses            ?      166           
  Partials          ?      113
Impacted Files Coverage Δ
db.go 80.51% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d3ab93...ea18f34. Read the comment docs.

@@ -687,9 +687,7 @@ func (db *DB) View(fn func(*Tx) error) error {
return err
}

if err := t.Rollback(); err != nil {
Copy link
Contributor

@heyitsanthony heyitsanthony Sep 12, 2017

Choose a reason for hiding this comment

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

I think the intent here is to avoid using unexported functions across object boundaries to avoid tight coupling.

It'd probably be better to have:

     return t.Rollback()
}

which would directly correspond with the Update code. Rollback is the method for closing read-only transactions; close is atypical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense to me. tx.close is tx's own function. And return t.Rollback() matches return t.Commit() in Update function.

I'll update this commit later.

Make return line of db.View corresponding with db.Update.
@lorneli lorneli changed the title db: close tx instead of calling Rollback in the end of View function db: return t.Rollback directly in the end of View function Sep 12, 2017
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

lgtm thanks

@xiang90
Copy link
Contributor

xiang90 commented Sep 12, 2017

lgtm

@xiang90 xiang90 merged commit a4199f8 into etcd-io:master Sep 12, 2017
@lorneli lorneli deleted the bbolt_db branch September 13, 2017 09:15
@gyuho gyuho changed the title db: return t.Rollback directly in the end of View function db.go: return t.Rollback directly in the end of View function Sep 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants