-
Notifications
You must be signed in to change notification settings - Fork 150
Scale mutation queue with multiple shards #1048
Changes from 9 commits
7173c8a
5437f9b
fc8c64f
055ac62
39122ea
0748802
2b398b2
6ebf36c
f6a17ed
b48a1c1
69e197c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,12 @@ func (e *miniEnv) Close() { | |
e.stopMockServer() | ||
} | ||
|
||
type fakeQueueAdmin struct{} | ||
|
||
func (*fakeQueueAdmin) AddShards(ctx context.Context, domainID string, shardIDs ...int64) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the receiver simply be taken by value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that would simplify things a bit. |
||
return nil | ||
} | ||
|
||
func TestCreateDomain(t *testing.T) { | ||
for _, tc := range []struct { | ||
desc string | ||
|
@@ -171,7 +177,7 @@ func TestCreateRead(t *testing.T) { | |
t.Fatalf("Failed to create trillian log server: %v", err) | ||
} | ||
|
||
svr := New(logEnv.Log, mapEnv.Map, logEnv.Admin, mapEnv.Admin, storage, vrfKeyGen) | ||
svr := New(logEnv.Log, mapEnv.Map, logEnv.Admin, mapEnv.Admin, storage, &fakeQueueAdmin{}, vrfKeyGen) | ||
|
||
for _, tc := range []struct { | ||
domainID string | ||
|
@@ -223,7 +229,7 @@ func TestDelete(t *testing.T) { | |
t.Fatalf("Failed to create trillian log server: %v", err) | ||
} | ||
|
||
svr := New(logEnv.Log, mapEnv.Map, logEnv.Admin, mapEnv.Admin, storage, vrfKeyGen) | ||
svr := New(logEnv.Log, mapEnv.Map, logEnv.Admin, mapEnv.Admin, storage, &fakeQueueAdmin{}, vrfKeyGen) | ||
|
||
for _, tc := range []struct { | ||
domainID string | ||
|
@@ -287,7 +293,7 @@ func TestListDomains(t *testing.T) { | |
t.Fatalf("Failed to create trillian log server: %v", err) | ||
} | ||
|
||
svr := New(logEnv.Log, mapEnv.Map, logEnv.Admin, mapEnv.Admin, storage, vrfKeyGen) | ||
svr := New(logEnv.Log, mapEnv.Map, logEnv.Admin, mapEnv.Admin, storage, &fakeQueueAdmin{}, vrfKeyGen) | ||
|
||
for _, tc := range []struct { | ||
domainIDs []string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ package google.keytransparency.sequencer; | |
import "google/protobuf/empty.proto"; | ||
|
||
message MapMetadata { | ||
// MapSourceSlice is the range of inputs that have been included in a map revision. | ||
// SourceSlice is the range of inputs that have been included in a map revision. | ||
message SourceSlice { | ||
// lowest_watermark is the lowest primary key (exclusive) the source log | ||
gdbelvin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// that has been incorporated into this map revision. The primary keys of | ||
|
@@ -36,8 +36,9 @@ message MapMetadata { | |
// of logged items MUST be monotonically increasing. | ||
int64 highest_watermark = 2; | ||
} | ||
// source defines the range of inputs used for this map revision. | ||
SourceSlice source = 1; | ||
reserved 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you at a point where you can't afford breaking compatibility by simply removing this field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fortunately we're early enough that we're happy to break compatibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that was my point. Why using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good habits I suppose :-) |
||
// sources defines the ranges of inputs used for this map revision for each slice. | ||
pav-kv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
map<int64, SourceSlice> sources = 2; | ||
} | ||
|
||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,11 +70,10 @@ func createMetrics(mf monitoring.MetricFactory) { | |
|
||
// Queue reads messages that haven't been deleted off the queue. | ||
type Queue interface { | ||
// HighWatermark returns the highest primary key in the mutations table for DomainID. | ||
HighWatermark(ctx context.Context, domainID string) (int64, error) | ||
// ReadQueue returns the messages between (low, high] for domainID. | ||
// TODO(gbelvin): Add paging API back in to support sharded reads. | ||
ReadQueue(ctx context.Context, domainID string, low, high int64) ([]*mutator.QueueMessage, error) | ||
// HighWatermark returns the highest timestamp in the mutations table. | ||
pav-kv marked this conversation as resolved.
Show resolved
Hide resolved
gdbelvin marked this conversation as resolved.
Show resolved
Hide resolved
gdbelvin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
HighWatermarks(ctx context.Context, domainID string) (map[int64]int64, error) | ||
// Read returns up to batchSize messages for domainID. | ||
gdbelvin marked this conversation as resolved.
Show resolved
Hide resolved
gdbelvin marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose that this is no longer really a I'll replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think yes. How about renaming the |
||
ReadQueue(ctx context.Context, domainID string, shard, low, high int64) ([]*mutator.QueueMessage, error) | ||
} | ||
|
||
// Server implements KeyTransparencySequencerServer. | ||
|
@@ -126,7 +125,7 @@ func (s *Server) RunBatch(ctx context.Context, in *spb.RunBatchRequest) (*empty. | |
if err := proto.Unmarshal(latestMapRoot.Metadata, &lastMeta); err != nil { | ||
return nil, err | ||
} | ||
high, err := s.queue.HighWatermark(ctx, in.DomainId) | ||
highs, err := s.queue.HighWatermarks(ctx, in.DomainId) | ||
if err != nil { | ||
return nil, status.Errorf(codes.Internal, "HighWatermark(): %v", err) | ||
} | ||
|
@@ -138,53 +137,55 @@ func (s *Server) RunBatch(ctx context.Context, in *spb.RunBatchRequest) (*empty. | |
// Count items to be processed. Unfortunately, this means we will be | ||
// reading the items to be processed twice. Once, here in RunBatch and | ||
// again in CreateEpoch. | ||
metadata := &spb.MapMetadata{ | ||
Source: &spb.MapMetadata_SourceSlice{ | ||
LowestWatermark: lastMeta.GetSource().GetHighestWatermark(), | ||
sources := make(map[int64]*spb.MapMetadata_SourceSlice) | ||
for sliceID, high := range highs { | ||
sources[sliceID] = &spb.MapMetadata_SourceSlice{ | ||
LowestWatermark: lastMeta.Sources[sliceID].GetHighestWatermark(), | ||
HighestWatermark: high, | ||
}, | ||
} | ||
} | ||
|
||
msgs, err := s.readMessages(ctx, in.DomainId, metadata.GetSource()) | ||
msgs, err := s.readMessages(ctx, in.DomainId, sources) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if int32(len(msgs)) < in.MinBatch { | ||
if len(msgs) < int(in.MinBatch) { | ||
gdbelvin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return &empty.Empty{}, nil | ||
} | ||
|
||
return s.CreateEpoch(ctx, &spb.CreateEpochRequest{ | ||
DomainId: in.DomainId, | ||
Revision: int64(latestMapRoot.Revision) + 1, | ||
MapMetadata: metadata, | ||
MapMetadata: &spb.MapMetadata{Sources: sources}, | ||
}) | ||
} | ||
|
||
func (s *Server) readMessages(ctx context.Context, domainID string, | ||
source *spb.MapMetadata_SourceSlice) ([]*ktpb.EntryUpdate, error) { | ||
// Read mutations | ||
batch, err := s.queue.ReadQueue(ctx, domainID, | ||
source.GetLowestWatermark(), source.GetHighestWatermark()) | ||
if err != nil { | ||
return nil, status.Errorf(codes.Internal, "ReadQueue(): %v", err) | ||
} | ||
msgs := make([]*ktpb.EntryUpdate, 0, len(batch)) | ||
for _, m := range batch { | ||
msgs = append(msgs, &ktpb.EntryUpdate{ | ||
Mutation: m.Mutation, | ||
Committed: m.ExtraData, | ||
}) | ||
sources map[int64]*spb.MapMetadata_SourceSlice) ([]*ktpb.EntryUpdate, error) { | ||
msgs := make([]*ktpb.EntryUpdate, 0) | ||
gdbelvin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for shardID, source := range sources { | ||
batch, err := s.queue.ReadQueue(ctx, domainID, shardID, | ||
source.GetLowestWatermark(), source.GetHighestWatermark()) | ||
if err != nil { | ||
return nil, status.Errorf(codes.Internal, "ReadQueue(): %v", err) | ||
} | ||
for _, m := range batch { | ||
msgs = append(msgs, &ktpb.EntryUpdate{ | ||
Mutation: m.Mutation, | ||
Committed: m.ExtraData, | ||
}) | ||
} | ||
} | ||
return msgs, nil | ||
} | ||
|
||
// CreateEpoch applies the supplied mutations to the current map revision and creates a new epoch. | ||
func (s *Server) CreateEpoch(ctx context.Context, in *spb.CreateEpochRequest) (*empty.Empty, error) { | ||
domainID := in.GetDomainId() | ||
if in.MapMetadata.GetSource() == nil { | ||
if in.MapMetadata.GetSources() == nil { | ||
return nil, status.Errorf(codes.InvalidArgument, "missing map metadata") | ||
} | ||
msgs, err := s.readMessages(ctx, in.DomainId, in.MapMetadata.GetSource()) | ||
msgs, err := s.readMessages(ctx, in.DomainId, in.MapMetadata.GetSources()) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is supposed to create shards if they are more than 1? This looks like a place for some TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue #1048
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably meant #1063
(putting it to create a link between github issues)