[api] Implement replace endpoint #1162

schallert · 2018-11-12T20:45:39Z

Adds an endpoint for replacing an instance across all services.

codecov · 2018-11-12T20:52:36Z

Codecov Report

Merging #1162 into master will decrease coverage by <.1%.
The diff coverage is 62.8%.

@@           Coverage Diff            @@
##           master   #1162     +/-   ##
========================================
- Coverage    71.5%   71.5%   -0.1%     
========================================
  Files         726     727      +1     
  Lines       60839   60905     +66     
========================================
+ Hits        43540   43566     +26     
- Misses      14523   14547     +24     
- Partials     2776    2792     +16

Flag	Coverage Δ
#aggregator	`81.6% <ø> (ø)`	⬆️
#cluster	`86% <ø> (ø)`	⬆️
#collector	`78.1% <ø> (ø)`	⬆️
#dbnode	`81.1% <ø> (-0.1%)`	⬇️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`75.2% <ø> (-0.1%)`	⬇️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`18.3% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (ø)`	⬆️
#query	`64.2% <62.8%> (-0.1%)`	⬇️
#x	`75.2% <ø> (ø)`	⬆️

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 a2fab37...0516e8a. Read the comment docs.

richardartoul · 2018-11-13T16:24:10Z

docs/operational_guide/placement_configuration.md

@@ -126,16 +126,31 @@ After sending the add command you will need to wait for the M3DB cluster to reac

 #### Removing a Node

-Send a DELETE request to the `/api/v1/placement/<NODE_ID>` endpoint.
+Send a DELETE request to the `/api/v1/services/m3db/placement/<NODE_ID>` endpoint.


nice thanks, I think the legacy stuff still works but good to update the docs to the new stuff

richardartoul · 2018-11-13T16:24:54Z

docs/operational_guide/placement_configuration.md


+```bash
+curl -X POST <M3_COORDINATOR_HOST_NAME>:<M3_COORDINATOR_PORT(default 7201)>/api/v1/services/m3db/placement/replace -d '{


Does this all work out correctly with various numbers of inputs and outputs? I.E if I want to replace one node with two, or remove two and add one etc

It should so long as that algorithm considers that change valid, since we don't do much rather than be a glorified wrapper around that. @cw9 can chime in on the algo stuff

this is fine, the algo basically calls the add() and then call the remove() on non sharded placements

richardartoul · 2018-11-13T16:26:09Z

src/query/api/v1/handler/placement/add_test.go

@@ -154,7 +154,7 @@ func TestPlacementAddHandler_SafeOK(t *testing.T) {
 		)
 		switch serviceName {
 		case M3AggregatorServiceName:
-			req = httptest.NewRequest(AddHTTPMethod, M3DBAddURL, strings.NewReader(`{"instances":[{"id": "host1","isolation_group": "rack1","zone": "test","weight": 1,"endpoint": "http://host1:1234","hostname": "host1","port": 1234}]}`))
+			req = httptest.NewRequest(AddHTTPMethod, M3AggAddURL, strings.NewReader(`{"instances":[{"id": "host1","isolation_group": "rack1","zone": "test","weight": 1,"endpoint": "http://host1:1234","hostname": "host1","port": 1234}]}`))


richardartoul · 2018-11-13T16:36:09Z

src/query/api/v1/handler/placement/replace.go

+		}
+	}
+
+	newPlacement, err := algo.ReplaceInstances(curPlacement, req.LeavingInstanceIDs, candidates)


can you add a comment about why you use the algo / check and set stuff

richardartoul · 2018-11-13T16:36:28Z

src/query/api/v1/handler/placement/replace.go

+		return nil, err
+	}
+
+	return newPlacement.SetVersion(version + 1), nil


A little annoying that CheckAndSet doesn't return the new version, but this is guaranteed to be correct right (version +1)?

It currently is but admittedly that's tied to the etcd implementation I think. The API CheckAndSet uses under the hood propagates the version but we drop that when the placement service does CheckAndSet.

We could (and probably should) refactor that but it would likely require updating a bunch of interface implementations. I can open an issue.

Hm, you could just do a get placement operation to avoid this possibly (i.e. just read what you wrote)?

Hey can you return perhaps just:

p, _, err := service.Placement() return p, err

Just so before we have some wrapper that does this for you in a way that can be reused, you at least just read back what you wrote?

That read after CAS breaks the atomicity, the current implementation is the only way to get atomicity in placement change with validation, a more proper solution is this

richardartoul · 2018-11-13T16:38:10Z

src/query/generated/assets/openapi/spec.yml

+      tags:
+      - "M3DB Placement"
+      - "M3DB"
+      summary: "Replace an M3DBNode"


do we concatenate M3DB and Node together in the other summaries?

Nope, will fix

Other M3DB ones:

summary: "Add an instance to the placement"

Maybe "Replace an instance in the placement"? shrug

richardartoul · 2018-11-13T16:41:38Z

src/query/api/v1/handler/placement/replace.go

+
+var (
+	// M3DBReplaceURL is the url for the m3db replace handler (method POST).
+	M3DBReplaceURL = path.Join(handler.RoutePrefixV1, M3DBServicePlacementPathName, replacePathName)


You wrote documentation for m3db/m3coordiantor/m3agg but only created a route for M3DB. Wanna create the other routes? Also we should get @cw9 to confirm there are no other special changes we need to make to support M3Agg replaces (when I did the m3agg stuff I looked at the r2admin code to see if there was anything special but Chao can probably just answer this question quickly)

You might need to change ServiceWithAlgo function to make sure it's using the same logic with r2admin, in there we had functions like GetOptionsForReplace, Richies work probably only moved GetOptions over.

@cw9 do we need more logic past what's here?

m3/src/query/api/v1/handler/placement/common.go

Line 262 in a2fab37

case M3AggregatorServiceName:

arnikola

LGTM w nits

src/query/api/v1/handler/placement/replace.go

justinjc · 2018-11-13T16:52:11Z

docs/operational_guide/placement_configuration.md

@@ -64,10 +64,10 @@ The instructions below all contain sample curl commands, but you can always revi

 #### Placement Initialization

-Send a POST request to the `/api/v1/placement/init` endpoint
+Send a POST request to the `/api/v1/services/m3db/placement/init` endpoint


I think right now it's a little ambiguous as to when to use /api/v1/... and when to use others like /api/v1/services/m3db/.... There are other mentions of /api/v1/placement like in docs/how_to (Github docs) and in our docker integration tests. It would be great to get some clarification on when to use which, but perhaps that's for a larger discussion in another PR.

cc @richardartoul

@justinjc It should always be the /services/ ones, we just kept the old names in to not make a breaking change. Anywhere we have a URL without the service name should just be updated to include it

README.md

src/query/api/v1/handler/placement/replace.go

robskillington

LGTM other than minor comment about perhaps just returning latest placement instead of manual incrementing the version?

[api] Implement replace endpoint

dc654cf

Adds an endpoint for replacing an instance across all services.

schallert requested review from cw9, justinjc and prateek November 12, 2018 20:52

schallert force-pushed the schallert/replace_api branch from acb76a8 to fda5c4b Compare November 12, 2018 21:45

add openapi spec

e132f84

schallert force-pushed the schallert/replace_api branch from fda5c4b to e132f84 Compare November 12, 2018 21:55

update docs

85841a3

richardartoul reviewed Nov 13, 2018

View reviewed changes

arnikola approved these changes Nov 13, 2018

View reviewed changes

justinjc reviewed Nov 13, 2018

View reviewed changes

robskillington reviewed Nov 13, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

robskillington reviewed Nov 13, 2018

View reviewed changes

src/query/api/v1/handler/placement/replace.go Outdated Show resolved Hide resolved

schallert added 2 commits November 13, 2018 12:15

agg+coord routes; isStateless check; nits

c60289b

update spec

68d95ba

robskillington approved these changes Nov 13, 2018

View reviewed changes

TODO issue comment

0516e8a

schallert merged commit 8fa049d into master Nov 13, 2018

schallert deleted the schallert/replace_api branch November 13, 2018 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[api] Implement replace endpoint #1162

[api] Implement replace endpoint #1162

schallert commented Nov 12, 2018 •

edited

Loading

codecov bot commented Nov 12, 2018 •

edited

Loading

richardartoul Nov 13, 2018

richardartoul Nov 13, 2018

schallert Nov 13, 2018

cw9 Nov 13, 2018

richardartoul Nov 13, 2018

richardartoul Nov 13, 2018

schallert Nov 13, 2018

richardartoul Nov 13, 2018 •

edited

Loading

schallert Nov 13, 2018

robskillington Nov 13, 2018 •

edited

Loading

robskillington Nov 13, 2018

cw9 Nov 13, 2018

richardartoul Nov 13, 2018

schallert Nov 13, 2018

robskillington Nov 13, 2018

richardartoul Nov 13, 2018

schallert Nov 13, 2018

cw9 Nov 13, 2018

schallert Nov 13, 2018

arnikola left a comment

justinjc Nov 13, 2018 •

edited

Loading

richardartoul Nov 13, 2018

robskillington left a comment


		```bash
		curl -X POST <M3_COORDINATOR_HOST_NAME>:<M3_COORDINATOR_PORT(default 7201)>/api/v1/services/m3db/placement/replace -d '{

[api] Implement replace endpoint #1162

[api] Implement replace endpoint #1162

Conversation

schallert commented Nov 12, 2018 • edited Loading

codecov bot commented Nov 12, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardartoul Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnikola left a comment

Choose a reason for hiding this comment

justinjc Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

schallert commented Nov 12, 2018 •

edited

Loading

codecov bot commented Nov 12, 2018 •

edited

Loading

richardartoul Nov 13, 2018 •

edited

Loading

robskillington Nov 13, 2018 •

edited

Loading

justinjc Nov 13, 2018 •

edited

Loading