[coordinator] Marshal return val of kv handler with jsonpb #3116

wesleyk · 2021-01-22T22:44:42Z

Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE

Does this PR require updating code package or user-facing documentation?:
NONE

arnikola · 2021-01-22T22:46:44Z

src/query/api/v1/handler/database/kvstore.go

+	oldProtoMarshalled := bytes.NewBuffer(nil)
+	if err := (&jsonpb.Marshaler{}).Marshal(oldProtoMarshalled, oldProto); err != nil {
+		return nil, fmt.Errorf("failed to marshal old proto: %w", err)
+	}


Hm actually should this error out if it couldn't get previous, or just log a warning? Otherwise potential you get stuck on a poisoned message and can't roll forward, e.g. if we change the format of the proto at some point?

hmm good call, will do

ah yeah there's already a similar message above

codecov · 2021-01-22T23:05:38Z

Codecov Report

Merging #3116 (8ecf524) into master (c74143f) will increase coverage by 0.0%.
The diff coverage is 50.0%.

@@           Coverage Diff           @@
##           master    #3116   +/-   ##
=======================================
  Coverage    72.2%    72.2%           
=======================================
  Files        1084     1084           
  Lines      100203   100206    +3     
=======================================
+ Hits        72436    72442    +6     
- Misses      22729    22731    +2     
+ Partials     5038     5033    -5

Flag	Coverage Δ
aggregator	`75.8% <ø> (ø)`
cluster	`84.8% <ø> (ø)`
collector	`84.3% <ø> (ø)`
dbnode	`78.7% <ø> (+<0.1%)`	⬆️
m3em	`74.4% <ø> (ø)`
m3ninx	`73.2% <ø> (+<0.1%)`	⬆️
metrics	`20.0% <ø> (ø)`
msg	`74.1% <ø> (+0.1%)`	⬆️
query	`67.2% <50.0%> (-0.1%)`	⬇️
x	`80.2% <ø> (-0.3%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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 c74143f...8ecf524. Read the comment docs.

arnikola

LGTM

* master: [dbnode] Handle empty slices in convert.FromSeriesIDAndEncodedTags (#3107) [dbnode] Fix AggregateQuery limits (#3112) [coordinator] Marshal return val of kv handler with jsonpb (#3116) [m3db] [m3coordinator] Properly pipe along require-exhaustive param for aggregate queries (#3115)

[coordinator] Marshal return val of kv handler with jsonpb

e9398c5

arnikola reviewed Jan 22, 2021

View reviewed changes

Feedback

8ecf524

arnikola approved these changes Jan 22, 2021

View reviewed changes

wesleyk merged commit 92bd2f4 into master Jan 22, 2021

wesleyk deleted the wesley-coord branch January 22, 2021 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[coordinator] Marshal return val of kv handler with jsonpb #3116

[coordinator] Marshal return val of kv handler with jsonpb #3116

wesleyk commented Jan 22, 2021

arnikola Jan 22, 2021

wesleyk Jan 22, 2021

wesleyk Jan 22, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

arnikola left a comment

[coordinator] Marshal return val of kv handler with jsonpb #3116

[coordinator] Marshal return val of kv handler with jsonpb #3116

Conversation

wesleyk commented Jan 22, 2021

arnikola Jan 22, 2021

Choose a reason for hiding this comment

wesleyk Jan 22, 2021

Choose a reason for hiding this comment

wesleyk Jan 22, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2021 • edited Loading

Codecov Report

arnikola left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 22, 2021 •

edited

Loading