Add GetLatestEpoch API #915

gdbelvin · 2018-01-26T12:56:52Z

The latest epoch is of interest in two use cases:

A client that is waiting to see if it's changes have made it into the next revision.
A gossiping peer that may not support gRPC streaming APIs.

By offering this API, clients will be able to efficiently poll just the Log Root until it changes before requesting a full proof for an updated entry. This is more much more efficient and would remove the need for a client to needlessly resubmit their UpdateEntry requests before a new epoch has been created.

codecov-io · 2018-01-26T13:01:38Z

Codecov Report

Merging #915 into master will decrease coverage by 0.36%.
The diff coverage is 22.58%.

@@            Coverage Diff             @@
##           master     #915      +/-   ##
==========================================
- Coverage   55.65%   55.29%   -0.37%     
==========================================
  Files          33       33              
  Lines        2280     2295      +15     
==========================================
  Hits         1269     1269              
- Misses        788      803      +15     
  Partials      223      223

Impacted Files	Coverage Δ
core/keyserver/keyserver.go	`33.83% <ø> (-1.74%)`	⬇️
core/keyserver/epochs.go	`37.08% <22.58%> (-1.81%)`	⬇️

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 0e33469...9b39df7. Read the comment docs.

Martin2112

Mostly suggestions. It's your repo so they may not all be appropriate.

Martin2112 · 2018-01-26T13:18:57Z

core/keyserver/epochs.go

 	})
 	if err != nil {
-		glog.Warningf("GetSignedMapRootByRevision(%v, %v): %v", domain.MapID, in.Epoch, err)
+		glog.Warningf("GetSignedMapRootByRevision(%v, %v): %v", d.MapID, revision, err)


This style of error message might make it hard to track down where they occurred. Might want to prefix the text with the rpc method name.

Martin2112 · 2018-01-26T13:21:51Z

core/keyserver/epochs.go

-			LogId: logID,
-		})
+// logProofs returns the proofs for a given epoch.
+func (s *Server) logProofs(ctx context.Context, d *domain.Domain, firstTreeSize int64, epoch int64) (*tpb.SignedLogRoot, *tpb.Proof, *tpb.Proof, error) {


Wonder if the proofs for an epoch could be wrapped into a single object for a cleaner signature.

That's a bit better.

Martin2112 · 2018-01-26T13:23:57Z

core/keyserver/keyserver.go

+// - LogRoot
+// - LogConsistency
+func (s *Server) getEntryByRevision(ctx context.Context, sth *tpb.SignedLogRoot, d *domain.Domain, userID, appID string, revision int64) (*pb.GetEntryResponse, error) {
+	if revision < int64(0) {


Is that cast required?

nope. Removed.

Martin2112 · 2018-01-26T13:25:11Z

core/keyserver/keyserver.go

 	logInclusion, err := s.tlog.GetInclusionProof(ctx,
 		&tpb.GetInclusionProofRequest{
-			LogId: domain.LogID,
+			LogId: d.LogID,
 			// SignedMapRoot must be placed in the log at MapRevision.
 			// MapRevisions start at 1. Log leaves start at 1.


Can you clarify meaning of "log leaves start at 1" here as leaf indices are 0 based.

The comment is incorrect. Updated:
MapRevisions start at 0. Log leaves start at 0.

Martin2112 · 2018-01-26T13:28:35Z

core/keyserver/keyserver.go

+}
+
+// latestRevision returns the latest map revision, given the latest sth.
+// The log is the authoritative source of the latest revision.


Naming seems a bit odd given it takes a log STH and returns a map revision. mapRevisionFor() maybe?

Martin2112 · 2018-01-26T13:31:43Z

core/keyserver/keyserver.go

 	}, nil
 }

 // ListEntryHistory returns a list of EntryProofs covering a period of time.
 func (s *Server) ListEntryHistory(ctx context.Context, in *pb.ListEntryHistoryRequest) (*pb.ListEntryHistoryResponse, error) {
 	// Lookup log and map info.
-	domain, err := s.domains.Read(ctx, in.DomainId, false)
+	domainID := in.GetDomainId()


GetDomainId() - should be GetDomainID().
Also looks like you often get the id then do domains.Read() on it, which might be a helper perhaps?

GetDomainId is a function generated from protobuf. The protobuf people decided to not follow Go's standard for initialisms unfortunately: golang/protobuf#156.

The emerging pattern is that all user facing functions accept domain_id and the first thing they do is domains.Read. Not sure what the best way to pull out a helper. It feels more like an interceptor that puts something in ctx, but that feels a little heavy handed and makes the code path harder to follow.

Ah, well it was just a suggestion.

gdbelvin · 2018-01-26T15:56:24Z

@Martin2112 thank you for the review!
I've pulled out the Catch log initialized errors commit into a separate PR: #916 and added unit tests. PTAL? Beers on me.

Refactor a few internal methods around for less copy-pasta.

gdbelvin · 2018-01-30T10:46:51Z

Now that #916 is in, this PR is a whole lot simpler!
PTAL

Martin2112 · 2018-01-30T10:55:51Z

core/keyserver/epochs.go

+	// Lookup log and map info.
+	d, err := s.domains.Read(ctx, in.DomainId, false)
+	if err != nil {
+		glog.Errorf("adminstorage.Read(%v): %v", in.DomainId, err)


Might want to prefix the errors with the method. Up to you.

Martin2112 · 2018-01-30T10:57:52Z

core/keyserver/epochs.go

+	return s.getEpochByRevision(ctx, d, in.GetFirstTreeSize(), currentEpoch)
+}
+
+// GetEpoch returns a the requested epoch.


gdbelvin requested review from phad, cesarghali and Martin2112 January 26, 2018 12:56

gdbelvin closed this Jan 26, 2018

Martin2112 reviewed Jan 26, 2018

View reviewed changes

gdbelvin reopened this Jan 26, 2018

gdbelvin force-pushed the f/GetLatestEpoch branch from ee38f27 to 94a2fa2 Compare January 26, 2018 16:31

gdbelvin added the feature label Jan 26, 2018

gdbelvin mentioned this pull request Jan 27, 2018

Intelligent Update strategy: replace blind Retry with WaitForSTH #917

Merged

gdbelvin added 4 commits January 30, 2018 10:39

Correct documentation

8d18136

Add GetLatestEpoch endpoint

318a1a0

Implement GetLatestEpoch

9c32d6f

Refactor a few internal methods around for less copy-pasta.

go generate ./core/api

59bbc31

gdbelvin force-pushed the f/GetLatestEpoch branch from 94a2fa2 to 59bbc31 Compare January 30, 2018 10:44

gdbelvin requested review from taknira and removed request for phad and cesarghali January 30, 2018 10:46

Martin2112 approved these changes Jan 30, 2018

View reviewed changes

nits

9b39df7

gdbelvin merged commit 444c2af into google:master Jan 30, 2018

gdbelvin deleted the f/GetLatestEpoch branch January 30, 2018 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetLatestEpoch API #915

Add GetLatestEpoch API #915

gdbelvin commented Jan 26, 2018 •

edited

Loading

codecov-io commented Jan 26, 2018 •

edited

Loading

Martin2112 left a comment

Martin2112 Jan 26, 2018

gdbelvin Jan 26, 2018

Martin2112 Jan 26, 2018

gdbelvin Jan 26, 2018

Martin2112 Jan 26, 2018

gdbelvin Jan 26, 2018

Martin2112 Jan 26, 2018

gdbelvin Jan 26, 2018

Martin2112 Jan 26, 2018

gdbelvin Jan 26, 2018

Martin2112 Jan 26, 2018

gdbelvin Jan 26, 2018 •

edited

Loading

Martin2112 Jan 26, 2018

gdbelvin commented Jan 26, 2018

gdbelvin commented Jan 30, 2018

Martin2112 Jan 30, 2018

gdbelvin Jan 30, 2018

Martin2112 Jan 30, 2018

gdbelvin Jan 30, 2018

Add GetLatestEpoch API #915

Add GetLatestEpoch API #915

Conversation

gdbelvin commented Jan 26, 2018 • edited Loading

codecov-io commented Jan 26, 2018 • edited Loading

Codecov Report

Martin2112 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin commented Jan 26, 2018

gdbelvin commented Jan 30, 2018

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

gdbelvin commented Jan 26, 2018 •

edited

Loading

codecov-io commented Jan 26, 2018 •

edited

Loading

gdbelvin Jan 26, 2018 •

edited

Loading