update and delete functions now take an optional rule-set-version #59

jskelcy · 2018-03-12T19:17:32Z

m3ctrl right now relies on tradition CRUD semantics for interacting with rules. With the current implementation, we are not able to do a check and set operation which are very powerful in a distributed systems setting. Right now the underlying storage layer of m3ctl is KV, which provides a check and set method for data with a given version. This diff updates the HTTP/JSON interface to accept a rule-set-version on rule updates and deletes so that we take advantage of KV check and sets.

NOTE:

Currently, if you query a single rule it does not return a rule-set-version. This means in order to update a rule and use check and set you would need to query the entire rule set, which does return a rule-set-version, then make your change with the version. I think it is reasonable to have those endpoints return a version number, however, it may be better to add those in a separate PR.
In all cases, I have not made the rule-set-version fields required. This will not break API backward compatibility. I am not sure the best way to go forward here. On one hand, we could say that if these fields are omitted then we skip the check and set, but that is potentially dangerous. The other options are to make it required and break back compat, and fix the UI, or create v2 endpoints. I am personally in the create a new version camp, as it will be much less complicated than making it optional while making the API cleaner. The final solution is to break back compat but that should be avoided whenever possible.

coveralls · 2018-03-12T19:23:29Z

Coverage remained the same at 57.333% when pulling db7f953 on js/version into c892bc2 on master.

m-sandusky · 2018-03-13T20:16:27Z

public/r2/v1/swagger/swagger.json

-                            "$ref": "#/definitions/MappingRule"
+                            "type": "object",
+                            "properties": {
+                                "rollup-rule": {


Should this be called mapping-rule?

m-sandusky · 2018-03-13T20:59:53Z

public/r2/v1/swagger/swagger.json

@@ -442,7 +464,7 @@
                    "200": {
                        "description": "Ruleset updated",
                        "schema": {
-                            "$ref": "#/definitions/ApiResponse"
+                            "$ref": "#/definitions/DeleteResponse"


A little confused about the DeleteResponse here - why do we need to return the version for this?

well, the version is for a rule set and this operation updates the current ruleset snapshot, there will be one less rule. If you then wanted to do further modifications after the delete you would be operating on a stale version of the rule set so it needs to be returned.

jskelcy · 2018-03-14T22:31:18Z

Updated this PR with a different approach. I have added a bulk update endpoint where a set of changes to a ruleset can be submitted. There is a flag which will apply all the updates either atomically or not based on the atomic flag. Initially, only atomic operations will be supported. This endpoint accepts a rule-set-verison which will be used as a check and set version number for the ruleset. If the version is stale an error will be returned from the endpoint.

saminzadeh · 2018-03-15T18:14:12Z

public/r2/v1/swagger/swagger.json

+                "parameters": [
+                    {
+                        "in": "path",
+                        "name": "namespaceID",


This is probably unnecessary to be required. You are updating the resource which is in the URL, so this can be inferred.

Interesting. I took a look at the other endpoint URIs and it looked like they required parameters that were part of the URL. Perhaps we should keep if for consistency.

If thats the case, then lets keep it consistent

saminzadeh · 2018-03-15T18:15:03Z

public/r2/v1/swagger/swagger.json

+                    {
+                        "in": "body",
+                        "name": "rule-set-version",
+                        "description": "The ruleset version to update, for check and set operations",


If this isn't passed, will it skip this check-set validation? If so, let's make a comment in the description here.

This should actually be required. I was thinking that if you did things non-atomically you would not need to pass this, but I was wrong about that, but it should always be required.

saminzadeh · 2018-03-15T18:28:46Z

public/r2/v1/swagger/swagger.json

+                            "$ref": "#/definitions/ApiResponse"
+                        }
+                    },
+                    "409": {


When would this occur? This is a PUT. This implies that we are creating something via a POST

A 409 is the case when you provide a rule-set-version that is stale.

Should the description clarify this?

I see. The description here is a little confusing. But I think a 409 is still relevant.

xichen2020 · 2018-03-15T20:05:19Z

public/r2/v1/swagger/swagger.json

+                    },
+                    {
+                        "in": "body",
+                        "name": "atomic",


Actually on a second thought, we probably don't need this to begin with and always require a ruleset version to start with. We can always relax that constraint later if needed.

xichen2020 · 2018-03-15T20:07:56Z

public/r2/v1/swagger/swagger.json

@@ -863,20 +936,80 @@
                    "type": "integer",
                    "format": "unixMillis"
                },
-                "mappingRules": {
+                "mappingRulesChanges": {


I think these should be mapping rules instead of mapping rule changes?

xichen2020 · 2018-03-15T20:08:04Z

public/r2/v1/swagger/swagger.json

                    "type": "array",
                    "items": {
                        "$ref": "#/definitions/MappingRule"
                    }
                },
-                "rollupRules": {
+                "rollupRulesChanges": {


I think these should be rollup rules instead of rollup rule changes?

I should have caught this. I think this was bad find and replace.

xichen2020 · 2018-03-18T19:47:18Z

public/r2/v1/swagger/swagger.json

@@ -84,7 +84,7 @@
                        }
                    },
                    "409": {
-                        "description": "Namespace already exists",
+                        "description": "The ruleset got updated while you were looking at it.",


Hm I thought we'd return 409 when the namespace already existed? If someone calls this API programmatically without checking whether the namespace exists beforehand, they can get 409 if they try to create a namespace that exists. If so, probably wanna keep it as "Namespace already exists"?

xichen2020 · 2018-03-18T19:50:37Z

public/r2/v1/swagger/swagger.json

@@ -184,6 +184,72 @@
                }
            }
        },
+        "/namespaces/{namespaceID}/ruleset/bulk": {
+            "put": {


Should this be POST rather than PUT since calling this endpoint twice with the same set of changes are not idempotent?

xichen2020 · 2018-03-18T19:53:24Z

public/r2/v1/swagger/swagger.json

+                "tags":[
+                    "namespaces"
+                ],
+                "summary": "Bulk update endpoint for namespace changes",


nit: for ruleset changes.

xichen2020 · 2018-03-18T19:54:30Z

public/r2/v1/swagger/swagger.json

+                    },
+                    {
+                        "in": "body",
+                        "name": "rule-set-version",


Should this be ruleset-version instead of rule-set-version?

xichen2020 · 2018-03-18T19:55:40Z

public/r2/v1/swagger/swagger.json

+                    {
+                        "in": "body",
+                        "name": "rule-set-version",
+                        "description": "The ruleset version to update, for check and set operations",


nit: might be more clear to say "The ruleset version to atomically apply bulk updates against"

xichen2020 · 2018-03-18T20:01:42Z

public/r2/v1/swagger/swagger.json

+            "properties": {
+                "operation": {
+                    "type": "string",
+                    "enum": ["add", "remove", "change"],


Does this mean we might add / delete namespace as part of the bulk operation? Might wanna think about if we want to allow that given that we already have APIs for adding new namespaces and deleting existing namespaces?

This was something I was thinking about. It does seem like this endpoint would be more simple if we did not allow for these types of operations, but if users are going to be hitting this endpoint programmatically it might be easier to have a one-stop endpoint for manipulating rules. I can remove it for now and if we need to add it back later we can.

xichen2020 · 2018-03-18T20:03:35Z

public/r2/v1/swagger/swagger.json

+                },
+                "from": {
+                    "$ref": "#/definitions/MappingRuleData",
+                    "description": "old version of rule"


Perhaps better to say "old rule data"?

Also this can be null for "add" and "change" operations I assume?

xichen2020 · 2018-03-18T20:03:54Z

public/r2/v1/swagger/swagger.json

+                },
+                "to": {
+                    "$ref": "#/definitions/MappingRuleData",
+                    "description": "new version of rule"


Perhaps better to say "new rule data"?

Also this can be null for "remove" operations I assume?

xichen2020 · 2018-03-18T20:04:05Z

public/r2/v1/swagger/swagger.json

+                },
+                "from": {
+                    "$ref": "#/definitions/RollupRuleData",
+                    "description": "old version of rule"


Perhaps better to say "old rule data"?

Also this can be null for "add" and "change" operations I assume?

xichen2020 · 2018-03-18T20:04:14Z

public/r2/v1/swagger/swagger.json

+                },
+                "to": {
+                    "$ref": "#/definitions/RollupRuleData",
+                    "description": "new version of rule"


Perhaps better to say "new rule data"?

Also this can be null for "remove" operations I assume?

xichen2020

LGTM.

xichen2020 · 2018-03-21T13:24:47Z

public/r2/v1/swagger/swagger.json

+            "properties": {
+                "operation": {
+                    "type": "string",
+                    "enum": ["add", "remove", "change"],


Can we please change remove to delete for consistency? That's what we use in m3metrics as well.

jskelcy force-pushed the js/version branch from a463e51 to 1b69769 Compare March 12, 2018 19:18

jskelcy requested review from saminzadeh, xichen2020 and m-sandusky March 12, 2018 19:38

m-sandusky reviewed Mar 13, 2018

View reviewed changes

jskelcy force-pushed the js/version branch 4 times, most recently from b9d9cf3 to 41dd2c3 Compare March 14, 2018 22:26

jskelcy force-pushed the js/version branch 3 times, most recently from 571ad04 to be710d3 Compare March 14, 2018 22:41

Update m3x, m3metrics (#57)

133a812

jskelcy force-pushed the js/version branch 3 times, most recently from fe30074 to 58c4aeb Compare March 14, 2018 23:15

saminzadeh reviewed Mar 15, 2018

View reviewed changes

xichen2020 reviewed Mar 15, 2018

View reviewed changes

jskelcy force-pushed the js/version branch 2 times, most recently from d099d1a to 6ab39b8 Compare March 16, 2018 13:24

saminzadeh approved these changes Mar 16, 2018

View reviewed changes

xichen2020 reviewed Mar 18, 2018

View reviewed changes

jskelcy force-pushed the js/version branch 2 times, most recently from d0b55ae to 1a51f57 Compare March 19, 2018 18:44

xichen2020 approved these changes Mar 21, 2018

View reviewed changes

xichen2020 reviewed Mar 21, 2018

View reviewed changes

add bulk update endpoint to swagger spec

db7f953

jskelcy force-pushed the js/version branch from 1a51f57 to db7f953 Compare March 21, 2018 14:16

jskelcy merged commit 856ed0e into master Mar 21, 2018

jskelcy deleted the js/version branch March 29, 2018 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update and delete functions now take an optional rule-set-version #59

update and delete functions now take an optional rule-set-version #59

jskelcy commented Mar 12, 2018 •

edited

coveralls commented Mar 12, 2018 •

edited

m-sandusky Mar 13, 2018

jskelcy Mar 13, 2018

m-sandusky Mar 13, 2018

jskelcy Mar 13, 2018

jskelcy commented Mar 14, 2018

saminzadeh Mar 15, 2018

jskelcy Mar 15, 2018

saminzadeh Mar 15, 2018

saminzadeh Mar 15, 2018

jskelcy Mar 15, 2018

saminzadeh Mar 15, 2018

jskelcy Mar 15, 2018

saminzadeh Mar 15, 2018

jskelcy Mar 15, 2018

xichen2020 Mar 15, 2018

xichen2020 Mar 15, 2018

xichen2020 Mar 15, 2018

jskelcy Mar 16, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

jskelcy Mar 19, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

xichen2020 Mar 18, 2018

xichen2020 left a comment

xichen2020 Mar 21, 2018

update and delete functions now take an optional rule-set-version #59

update and delete functions now take an optional rule-set-version #59

Conversation

jskelcy commented Mar 12, 2018 • edited

coveralls commented Mar 12, 2018 • edited

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

jskelcy commented Mar 14, 2018

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

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

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

xichen2020 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskelcy commented Mar 12, 2018 •

edited

coveralls commented Mar 12, 2018 •

edited