Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

Replace a Map of LogSources with a List of LogSources #1124

Merged
merged 12 commits into from
Nov 26, 2018

Conversation

gdbelvin
Copy link
Contributor

A list of []*spb.MapMetadata_SourceSlice is a lot easier to manage and iterate through than map[int64]*spb.MapMetadata_SourceSlice.

  • The list of sources going into a map does need to be one contiguous range per log. One can imagine ingesting multiple, non-contiguous ranges from a single log too.
  • It has a defined order for iteration which is useful when we are paging.
  • Lists can be natively emitted by beam.CreateList, unlike KeyValue pairs.
  • We can avoid creating new types SourcesEntry which are not protos and therefore will have trouble when it comes to serializing them for a map reduce.

core/sequencer/server_test.go Show resolved Hide resolved
core/sequencer/server_test.go Show resolved Hide resolved
core/sequencer/server_test.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 23, 2018

Codecov Report

Merging #1124 into master will decrease coverage by 0.08%.
The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1124      +/-   ##
==========================================
- Coverage   67.01%   66.92%   -0.09%     
==========================================
  Files          37       37              
  Lines        2689     2670      -19     
==========================================
- Hits         1802     1787      -15     
+ Misses        583      581       -2     
+ Partials      304      302       -2
Impacted Files Coverage Δ
core/keyserver/paginator.go 93.93% <100%> (+1.21%) ⬆️
core/keyserver/revisions.go 63.92% <83.33%> (+0.22%) ⬆️
core/sequencer/server.go 65.35% <96.15%> (+1.19%) ⬆️

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 4930b71...ed84c49. Read the comment docs.

@gdbelvin gdbelvin requested review from jtoohill and pav-kv November 23, 2018 16:02
* 'meta' of github.com:gdbelvin/keytransparency:
  Read map at a specific revision (google#1118)
@gdbelvin
Copy link
Contributor Author

beam.Run also refuses to serialize map[int64]*spb.MapMetadata_SourceSlice because it is not a proto.

@gdbelvin gdbelvin added the for review PR is ready for review label Nov 26, 2018
Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

Huge +1 to this change.
Flushing some initial comments, and I'm still reading it.

core/keyserver/paginator.go Outdated Show resolved Hide resolved
core/keyserver/paginator.go Outdated Show resolved Hide resolved
core/keyserver/revisions.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

nits

core/sequencer/server.go Outdated Show resolved Hide resolved
core/sequencer/server.go Outdated Show resolved Hide resolved
core/keyserver/paginator.go Outdated Show resolved Hide resolved
core/keyserver/paginator.go Outdated Show resolved Hide resolved
@pav-kv
Copy link
Contributor

pav-kv commented Nov 26, 2018

There are some other "shard" mentionings left from the previous code. Find&replace them with "slice index" to be consistent please.

* master:
  Define LogMapFn as it currently exists (google#1126)
Copy link
Contributor

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

LGTM % supernits.

core/keyserver/paginator.go Outdated Show resolved Hide resolved
core/keyserver/paginator.go Outdated Show resolved Hide resolved
* master:
  Simplify the Reduce function interface (google#1127)
@gdbelvin gdbelvin merged commit a85af34 into google:master Nov 26, 2018
@gdbelvin gdbelvin deleted the meta branch November 26, 2018 18:57
gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Nov 26, 2018
* master:
  Replace a Map of LogSources with a List of LogSources (google#1124)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
for review PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants