-
Notifications
You must be signed in to change notification settings - Fork 453
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
Add endpoint to set database bootstrappers dynamically #1239
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1239 +/- ##
========================================
- Coverage 70.7% 70.7% -0.1%
========================================
Files 748 750 +2
Lines 63078 63136 +58
========================================
+ Hits 44644 44672 +28
- Misses 15553 15573 +20
- Partials 2881 2891 +10
Continue to review full report at Codecov.
|
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.
LGTM
return | ||
} | ||
|
||
array := new(commonpb.StringArrayProto) |
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 tested this right? It will allocate the underlying slice properly?
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.
Yeah, this works. Unmarshal handles that.
value, err := store.Get(kvconfig.BootstrapperKey) | ||
if err != nil { | ||
logger.Error("unable to get kv key", zap.Any("error", err)) | ||
xhttp.Error(w, err, http.StatusInternalServerError) |
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.
is this a 500? Seems like its basically the equivalent of a 404 or even a 200 with a null response or something
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.
I'll check if its ErrNotFound.
return nil, xhttp.NewParseError(fmt.Errorf("no values"), http.StatusBadRequest) | ||
} | ||
|
||
if err := dbconfig.ValidateBootstrappersOrder(array.Values); err != nil { |
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.
nice, monorepo ftw
tags: | ||
- "M3DB Database" | ||
- "M3DB" | ||
summary: "Configure M3DB set bootstrappers dynamically, which override config" |
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.
"Dynamically override the bootstrappers configured in M3DBs node-level configuration"
tags: | ||
- "M3DB Database" | ||
- "M3DB" | ||
summary: "Configure M3DB get bootstrappers dynamically, which override config" |
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.
"Retrieve the dynamically configured bootstrappers override, if any."
No description provided.