[storage/mysql] Tolerate gaps in GetLeavesByRange#1061
Conversation
storage/mysql/log_storage.go
Outdated
| } | ||
|
|
||
| func (t *logTreeTX) GetLeavesByRange(ctx context.Context, start, count int64) ([]*trillian.LogLeaf, error) { | ||
| // TODO(pavelkalinnikov): Clip `count`, for example to max(count, 64k). |
There was a problem hiding this comment.
nit: min(count, 64k)
Codecov Report
@@ Coverage Diff @@
## master #1061 +/- ##
==========================================
- Coverage 62.61% 62.57% -0.05%
==========================================
Files 103 103
Lines 8448 8455 +7
==========================================
+ Hits 5290 5291 +1
- Misses 2630 2634 +4
- Partials 528 530 +2
Continue to review full report at Codecov.
|
storage/mysql/log_storage.go
Outdated
|
|
||
| if len(ret) == 0 { | ||
| return nil, status.Errorf(codes.InvalidArgument, "no leaves found in range [%d, %d+%d)", start, start, count) | ||
| return nil, status.Errorf(codes.InvalidArgument, "no contiguous prefix of leaves found in [%d, %d+%d)", start, start, count) |
There was a problem hiding this comment.
I have an impression this shouldn't be an error. For example, if the request is (start=treeSize, count=100), looks reasonable to return an empty slice?
There was a problem hiding this comment.
Also, this it probably not InvalidArgument anymore.
|
For safety can we limit this change to PREORDERED_LOG trees? It will probably need a small tweak to retain the tree type in the tx. |
|
I am not sure what you mean by safety in this case? I think we should rather make the API consistent regardless of the tree type. How about the following:
Since normally |
|
If there are gaps in a LOG tree somehow then it's probably too late to save it but the server should detect it. Alternatively, we could enforce that a log tree can never request a range outside of the tree and the not worry about gaps. Possibly the CTFE checks on the response are sufficient but we should try to exclude the possibility of bad data propagating if we can. |
|
Maybe also cap |
|
We should get the tree type via the trillian.Tree when the transaction starts. |
|
@Martin2112 Improved safety a bit, PTAL. |
storage/mysql/log_storage.go
Outdated
| func (t *logTreeTX) GetLeavesByRange(ctx context.Context, start, count int64) ([]*trillian.LogLeaf, error) { | ||
| if count <= 0 { | ||
| return nil, fmt.Errorf("invalid count %d", count) | ||
| return nil, fmt.Errorf("count=%d,want>0", count) |
There was a problem hiding this comment.
Not sure about changing these. I think they might get returned in server responses but not 100% certain.
There was a problem hiding this comment.
Yes, it gets returned in server responses. Should I leave it intact? I thought a bit clarified messages would be better.
There was a problem hiding this comment.
Made it slightly more human readable.
|
I don't really mind. This change is more Go style but users might be using
another language to make RPCs.
…On 20 March 2018 at 16:23, Pavel Kalinnikov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In storage/mysql/log_storage.go
<#1061 (comment)>:
> @@ -567,11 +567,26 @@ func (t *logTreeTX) GetLeavesByIndex(ctx context.Context, leaves []int64) ([]*tr
func (t *logTreeTX) GetLeavesByRange(ctx context.Context, start, count int64) ([]*trillian.LogLeaf, error) {
if count <= 0 {
- return nil, fmt.Errorf("invalid count %d", count)
+ return nil, fmt.Errorf("count=%d,want>0", count)
Yes, it gets returned in servers responses. Should I leave it intact? I
thought a bit clarified messages would be better.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1061 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AMv2T_h2arvYe7SaR8-kdJL4Hf73LGtaks5tgS0cgaJpZM4SwYIL>
.
|
PREORDERED_LOGusesGetLeavesByRangeto load sequenced entries beyond the tree size. It is a normal situation when those entries are not contiguous (AddSequencedLeavesdoes not enforce the order). This PR makesGetLeavesByRangereturn a shorter range in such scenario, instead of an error.