R4R: Don't acquire lock on read-only keybase #2555

alessio · 2018-10-22T21:31:16Z

Expose calls to acquire non-blocking read lock on LevelDB stores.
Change GetKeyBase() so that it opens stores in read-only mode by default.
Add GetKeyBaseWithWritePerm() to acquire write locks on stores.

Closes: #1255

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…llelization between gaiacli

codecov · 2018-10-22T22:20:49Z

Codecov Report

Merging #2555 into develop will decrease coverage by 0.05%.
The diff coverage is 25%.

@@             Coverage Diff             @@
##           develop    #2555      +/-   ##
===========================================
- Coverage    60.32%   60.27%   -0.06%     
===========================================
  Files          150      150              
  Lines         8613     8591      -22     
===========================================
- Hits          5196     5178      -18     
  Misses        3069     3069              
+ Partials       348      344       -4

alexanderbez

Thanks @alessio! I left some minor feedback. Also, how does this relate to #2489? Can we close one of these?

PENDING.md

alexanderbez · 2018-10-23T13:17:12Z

client/keys/add.go

@@ -63,7 +63,7 @@ func runAddCmd(cmd *cobra.Command, args []string) error {
 			return errMissingName()
 		}
 		name = args[0]
-		kb, err = GetKeyBase()
+		kb, err = GetKeyBaseWithWritePerm()


It seems that the old GetKeyBase is now the one that returns a KeyBase in RO mode. May I suggest renaming GetKeyBaseWithWritePerm to GetWriteKeyBase?

My initial approach was like yours, e.g. "oh, this is quite an API breakage - semantic changed radically". Though I must admit that I quite like the WithWritePerm suffix. It makes clear that you are getting a dB in write mode.

Agree with @alessio here.

client/keys/utils.go

ValarDragon · 2018-10-23T16:34:53Z

client/keys/utils.go

+
+// GetKeyBaseWithWritePerm initialize a keybase based on the configuration with write permissions.
+func GetKeyBaseWithWritePerm() (keys.Keybase, error) {
+	rootDir := viper.GetString(cli.HomeFlag)


Feel free to declare this out of scope of this PR, but it would be super nice to remove this function entirely and instead use GetKeyBaseFromDir variants (probably renamed to remove the FromDir.

I like the API exposed here. It explicitly mentions the permissions you are getting when you open the database.

The point I was making was removing the viper global variable, in favor of always explicitly passing in the rootDir, not about the permission itself.

Agreed, I guess it can be a separate PR though.

@ValarDragon I agree, let's do it in a separate PR, shall we?

Co-Authored-By: alessio <quadrispro@ubuntu.com>

alexanderbez

utACK -- thanks @alessio

cwgoes

Minor comment.

cwgoes · 2018-10-23T18:57:08Z

crypto/keys/keybase.go

@@ -426,6 +426,11 @@ func (kb dbKeybase) Update(name, oldpass string, getNewpass func() (string, erro
 	}
 }

+// CloseDB releases the lock and closes the storage backend.
+func (kb dbKeybase) CloseDB() {


Should we call this (e.g. in the code) in places other than the testcase you added? Seems like maybe so...

cwgoes · 2018-10-23T18:57:28Z

client/keys/utils.go

+
+// GetKeyBaseWithWritePerm initialize a keybase based on the configuration with write permissions.
+func GetKeyBaseWithWritePerm() (keys.Keybase, error) {
+	rootDir := viper.GetString(cli.HomeFlag)


Agreed, I guess it can be a separate PR though.

cwgoes

utACK, let's split out calling Close() and removing the Viper global usage to a second PR

ackratos and others added 2 commits October 20, 2018 20:26

#1255 make keybase opened with readonly option to support better para…

6c623b2

…llelization between gaiacli

Merge review comments/changes

add15b5

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners October 22, 2018 21:31

alessio added the ready-for-review label Oct 22, 2018

alessio mentioned this pull request Oct 22, 2018

[R4R] #1255 make keybase opened with readonly option for query-purpose cli commands #2489

Merged

5 tasks

Add tests

44a6c21

ackratos approved these changes Oct 23, 2018

View reviewed changes

alessio added refactor and removed refactor labels Oct 23, 2018

alessio requested review from alexanderbez and jackzampolin October 23, 2018 10:19

alexanderbez suggested changes Oct 23, 2018

View reviewed changes

ValarDragon reviewed Oct 23, 2018

View reviewed changes

alexanderbez and others added 2 commits October 23, 2018 10:43

Update PENDING.md

990852a

Co-Authored-By: alessio <quadrispro@ubuntu.com>

Update client/keys/utils.go

450873d

Co-Authored-By: alessio <quadrispro@ubuntu.com>

alexanderbez approved these changes Oct 23, 2018

View reviewed changes

cwgoes reviewed Oct 23, 2018

View reviewed changes

cwgoes approved these changes Oct 23, 2018

View reviewed changes

cwgoes merged commit a231bd8 into develop Oct 23, 2018

cwgoes deleted the alessio/1255-keybase-readonly branch October 23, 2018 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Don't acquire lock on read-only keybase #2555

R4R: Don't acquire lock on read-only keybase #2555

alessio commented Oct 22, 2018 •

edited

Loading

codecov bot commented Oct 22, 2018 •

edited

Loading

alexanderbez left a comment

alexanderbez Oct 23, 2018

alessio Oct 23, 2018

jackzampolin Oct 23, 2018

ValarDragon Oct 23, 2018 •

edited

Loading

jackzampolin Oct 23, 2018

ValarDragon Oct 23, 2018

alexanderbez Oct 23, 2018

cwgoes Oct 23, 2018

alessio Oct 23, 2018

alexanderbez left a comment

cwgoes left a comment

cwgoes Oct 23, 2018

alessio Oct 23, 2018

cwgoes Oct 23, 2018

cwgoes left a comment

R4R: Don't acquire lock on read-only keybase #2555

R4R: Don't acquire lock on read-only keybase #2555

Conversation

alessio commented Oct 22, 2018 • edited Loading

codecov bot commented Oct 22, 2018 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alessio commented Oct 22, 2018 •

edited

Loading

codecov bot commented Oct 22, 2018 •

edited

Loading

ValarDragon Oct 23, 2018 •

edited

Loading