Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid log fatal #466

Merged
merged 2 commits into from
Dec 1, 2020
Merged

avoid log fatal #466

merged 2 commits into from
Dec 1, 2020

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Nov 30, 2020

No description provided.

Signed-off-by: Ignacio Hagopian <[email protected]>
@jsign jsign self-assigned this Nov 30, 2020
Comment on lines +23 to 44
// @todo: Clean up tag numbers with next major release
message NewDBRequest {
bytes dbID = 1;
repeated CollectionConfig collections = 2;
string name = 3;
bool block = 5;
bytes threadKey = 6;
bytes key = 6;
bytes logKey = 7;
string name = 3;
repeated CollectionConfig collections = 2;

bool block = 5 [deprecated = true];
}

// @todo: Clean up tag numbers with next major release
message NewDBFromAddrRequest {
bytes addr = 1;
bytes key = 2;
repeated CollectionConfig collections = 3;
bytes logKey = 7;
string name = 4;
repeated CollectionConfig collections = 3;
bool block = 5;
bytes threadKey = 6;
bytes logKey = 7;

bytes threadKey = 6 [deprecated = true];
}
Copy link
Member

Choose a reason for hiding this comment

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

Somehow I missed this in Daniel's PR. threadKey was added, but we already have key. This change does not modify the proto tag numbers. We can clean this up on the next major release.

db.WithNewManagedToken(token),
db.WithNewManagedBackfillBlock(req.Block),
Copy link
Member

Choose a reason for hiding this comment

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

This has no meaning in the context of NewDB

return nil, err
}
if args.Key.Defined() && !args.Key.CanRead() {
return nil, ErrThreadReadKeyRequired
Copy link
Member

Choose a reason for hiding this comment

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

Early out if thread key does not contain a read component.

@sanderpick sanderpick merged commit 1e36eea into master Dec 1, 2020
@sanderpick sanderpick deleted the jsign/notfatal branch December 1, 2020 16:30
@jtacoma
Copy link

jtacoma commented Dec 8, 2020

FYI, this changed the public API: db.WithNewThreadKey was renamed to db.WithNewKey, and db.WithNewManagedThreadKey was renamed to db.WithNewManagedKey.

jtacoma added a commit to google/note-maps that referenced this pull request Dec 8, 2020
@sanderpick
Copy link
Member

Doh, thanks for the heads up @jtacoma - I'll make a not on the release that includes those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants