MANTA-4341 Optimize conditional object operations #47

chudley · 2020-03-12T14:20:24Z

This change gives buckets-mdapi the option of handling conditional requests itself for the getobject, createobject, updateobject, and deleteobject RPCs. I attempted to mirror the existing behaviour from buckets-api (which I think is based on Restify's approach) where possible. I'm happy to revisit any of that if we're OK with API changes that might make sense here.

There's a few parts in this change that I'm not totally sure about, but I wanted to get some eyes on it while it's being finished up. I still would also like to add more tests to tests/rpc_handlers.rs, and I'm sure there's some Rust-related no-nos I've committed. I also haven't actually verified that the transactions at the database level are working as I expect (I would like to include this in the test suite but I'm not sure what that would look like).

Probably my biggest concern is that the conditionals here aren't strictly following HTTP's definitions, such as error codes and supported conditions for certain operations (e.g. "If-Modified-Since" is supported on "updateobject" RPCs, but HTTP says it should only be used on GETs and HEADs). That enforcement happens in TritonDataCenter/manta-buckets-api#53 by way of selectively passing these conditional headers to buckets-mdapi.

This is on purpose for now, at least because of the precondition failures that would lead to a 304 Not Modified, which isn't a HTTP error but a redirect, which buckets-api handles as a successful response. It also helps with the CreateBucketObject case, where having the initial "getobject" in that request fail with the same conditions that the subsequent "createobject" would fail on is nice. I'd be interested to hear thoughts on this, and I also plan on documenting this somewhere with whatever conclusion we reach.

The change is make check clean. I've also written some new tests for node-manta that flex these conditional requests. They can be seen here.

A few examples of the getobject RPC using this new (optional) conditions object:

No conditions object

./target/debug/examples/fastcall --method getobject --args '[ { "bucket_id": "852e53fb-d09b-4b37-844c-d06a497d215f", "owner": "f55a289e-d1a4-476b-bc3f-6e15cd8e26d2", "name": "testing", "vnode": 0, "request_id": "bd0b716a-2873-4d75-a3d4-13f30c2c850b" } ]' | json
[
  {
    "bucket_id": "852e53fb-d09b-4b37-844c-d06a497d215f",
    "content_length": 0,
    "content_md5": "test",
    "content_type": "application/json",
    "created": "2019-10-24T08:14:21.945237Z",
    "headers": {},
    "id": "d2420c61-9bec-406c-a0e2-b992dfdbc0a9",
    "modified": "2019-10-24T08:14:21.945237Z",
    "name": "testing",
    "owner": "f55a289e-d1a4-476b-bc3f-6e15cd8e26d2",
    "properties": null,
    "sharks": [
      {
        "datacenter": "test-0",
        "manta_storage_id": "1.stor"
      }
    ]
  }
]

if-match success

./target/debug/examples/fastcall --method getobject --args '[ { "bucket_id": "852e53fb-d09b-4b37-844c-d06a497d215f", "owner": "f55a289e-d1a4-476b-bc3f-6e15cd8e26d2", "name": "testing", "vnode": 0, "request_id": "bd0b716a-2873-4d75-a3d4-13f30c2c850b", "conditions": { "if-match": [ "d2420c61-9bec-406c-a0e2-b992dfdbc0a9" ] } } ]' | json
[
  {
    "bucket_id": "852e53fb-d09b-4b37-844c-d06a497d215f",
    "content_length": 0,
    "content_md5": "test",
    "content_type": "application/json",
    "created": "2019-10-24T08:14:21.945237Z",
    "headers": {},
    "id": "d2420c61-9bec-406c-a0e2-b992dfdbc0a9",
    "modified": "2019-10-24T08:14:21.945237Z",
    "name": "testing",
    "owner": "f55a289e-d1a4-476b-bc3f-6e15cd8e26d2",
    "properties": null,
    "sharks": [
      {
        "datacenter": "test-0",
        "manta_storage_id": "1.stor"
      }
    ]
  }
]

if-match failure

./target/debug/examples/fastcall --method getobject --args '[ { "bucket_id": "852e53fb-d09b-4b37-844c-d06a497d215f", "owner": "f55a289e-d1a4-476b-bc3f-6e15cd8e26d2", "name": "testing", "vnode": 0, "request_id": "bd0b716a-2873-4d75-a3d4-13f30c2c850b", "conditions": { "if-match": [ "xxx" ] } } ]' | json
[
  {
    "error": {
      "message": "if-match '\"xxx\"' didn't match etag 'd2420c61-9bec-406c-a0e2-b992dfdbc0a9'",
      "name": "PreconditionFailedError"
    }
  }
]

if-modified-since failure

./target/debug/examples/fastcall --method getobject --args '[ { "bucket_id": "852e53fb-d09b-4b37-844c-d06a497d215f", "owner": "f55a289e-d1a4-476b-bc3f-6e15cd8e26d2", "name": "testing", "vnode": 0, "request_id": "bd0b716a-2873-4d75-a3d4-13f30c2c850b", "conditions": { "if-modified-since": "2020-03-12T10:00:00Z" } } ]' | json
[
  {
    "error": {
      "message": "object was modified at '2019-10-24T08:14:21.945237+00:00'; if-modified-since '2020-03-12T10:00:00+00:00'",
      "name": "PreconditionFailedError"
    }
  }
]

…d returning a Value. feels a little better.

kellymclaughlin

I haven't gone over everything yet, but I wanted to share my comments thus far just to give you some early feedback. This is looking great btw!

kellymclaughlin · 2020-03-19T23:08:58Z

buckets-mdapi/src/conditional.rs

+    pub if_unmodified_since: Option<types::Timestamptz>,
+}
+
+pub fn error(error_type: error::BucketsMdapiErrorType, msg: String) -> serde_json::Value {


It looks like this is expected to be a specialization for this module. I don't see it used anywhere else, but I may have just missed it. If that's true then maybe it makes sense to specialize it a bit further here to the BucketsMdapiErrorType::PreconditionFailedError type rather than having that parameterized. It also probably doesn't need to be pub if it's only used in this module.

Yea good point. I've changed this to be an internal function now that always returns a PreconditionFailedError. It's a bit thin on the ground with the changes to BucketsMdapiError as well, and I'm not totally sure on its utility any more. Thinking about it, it could probably just do with being removed

kellymclaughlin · 2020-03-20T14:59:39Z

buckets-mdapi/src/object/get.rs

+    .and_then(|_rows| {
+        // XXX
+        //
+        // Somehow this should skip this second get if the above conditional already gets the


Instead of doing the conditional::request first if we first do this query for the object since the SQL used is the same and then apply the conditional checks if appropriate afterwards in the and_then block that follows. I think that would work to get the same result, but avoid the extra query. What do you think?

I started looking at this, but I wasn't totally keen on having to also check for 0 or >1 rows in do_get, similar to what conditional::request has to do.

I've made some changes in fe7e575 that causes conditional::request to return the rows it finds in its query, and then go_get will just pass those back if there are Some(rows). Otherwise, None causes go_get to do the query itself (which would be the case if the request wasn't conditional).

I think it's doing the right thing, but I'd appreciate eyes on it!

kellymclaughlin · 2020-03-20T16:13:59Z

buckets-mdapi/src/object/get.rs

+             * Is reaching into the Value like this safe?  I think we get `Value::Null` on a bad
+             * value, so possibly it's ok.  Is it fast enough?
+             */
+            if e["name"] == "PostgresError" {


I think we could possibly improve on this a bit and not have to deal with the raw Value here. What I'm thinking is maybe we can have do_get return a Result<Option<ObjectResponse>, BucketsMdapiErrorType> rather than Result<Option<ObjectResponse>, Value>. If we extend BucketsMdapiErrorTyep with a new variant that looks like PostgresError(String) maybe we can achieve similar results without the need to muck directly with that Value. This is pretty hand wavy at this point so if you want I can try this out and share more details about what I mean if it seems like it will work.

This one ended up being a bit of a mission! I agree though because the error type was a little bit confusing for me at first.

Basically I've made enum BucketsMdapiErrorType into our internal representation of an error, and it's now enum BucketsMdapiError. Like you've said, we've now got String variants that include a message with the error which is nice. Only when it comes to serializing and deserializing do we wrap this into BucketsMdapiWrappedError, which buys us some niceties from Serde with wrapping it up into what Fast expects our errors to look like. I've kind of just inverted the existing BucketsMdapiErrorType and BucketsMdapiError types.

I hope that makes sense. It took me a while to get to this point because Serde can almost afford us to do away with ToString (except for our test suite) and BucketsMdapiWrappedError. I've written that up into a comment.

The delta for just this change is between fe7e575 and 1ceb15b.

Again, I think this is doing what I expect, but I'd appreciate extra eyes on it.

kellymclaughlin · 2020-03-20T16:23:37Z

buckets-mdapi/src/conditional.rs

+    })
+}
+
+fn is_conditional(conditions: &Conditions) -> bool {


For is_conditional and check_conditional you could consider making them methods attached to the Conditions type just to enable a call syntax of conditions.is_conditional() or conditions.check(). Doesn't affect functionality at all so feel free to ignore.

I like it, thanks! check and is_conditional are now methods of the Conditions type.

…t something for the 'double get' path.

kellymclaughlin · 2020-03-25T16:53:10Z

buckets-mdapi/src/object/get.rs

+         * return those rows as-is.
+         */
+        if let Some(r) = rows {
+            return Ok(r);


One caution here is that this would bail out early without committing the transaction which would result in a rollback. It doesn't matter in this case since we're only reading data out. You could actually just avoid using an explicit transaction for do_get because we're only doing the single query so there won't be any transaction id savings by explicitly managing a transaction.

Ah, didn't plan on that. I thought the rollback happened only when txn gets dropped at the end of do_get if txt.commit() hasn't been called, and that my early return would still lead to that commit happening. Maybe I've misunderstood this a bit. Mind pointing out where the rollback happens?

You're right though on the whole transaction handling thing. Your initial recommendation is probably the way to go. My main problem with having go_get check the conditions itself was that it would also have to check the row count for 0 or >1 rows, but maybe if I have Conditions::check() take RowSlice instead I can have that method do that row count check also. I'll have a little play around!

Ok, I've taken another go at this in chudley@17b9772. do_get now just runs the SQL for a get and passes an ObjectResponse to the conditional check, which has also been changed to accept that instead.

Yeah, I like the way that looks now. Very nice!

buckets-mdapi/src/error.rs

buckets-mdapi/src/sql.rs

kellymclaughlin · 2020-03-31T23:51:44Z

I think the CI checks failed just because it needs a merge from master.

chudley · 2020-04-02T17:33:34Z

Looks like checks are passing for this now after the merge from master.

I wanted to check whether this could be integrated ahead of all the buckets-api parts. It looks like that's the case, where the new node-manta tests in TritonDataCenter/node-manta#382 around conditional requests all pass except for the 400 Bad Request ones, which is for a bad date being supplied (as expected, noted in MANTA-4341).

So subject to review, we should be able to integrate this and followup later with the remaining up-stack pieces that are also in review.

Richard Bradley and others added 30 commits January 15, 2020 12:21

first pass at where these conditionals might live.

e92cef2

stubbing.

e6e9d7f

more stubbing.

7d1a084

more thinking.

433d56a

skip returning rows for now so I can better reason about this stuff.

61caeeb

try to streamline conditional signature. don't think it worked.

e609be6

jotting thoughts.

e0ab040

finally got some mix of errors being returned by the conditional call.

a97a389

i think precondition errors are passing through now.

69c0a50

breaking out precondition stuff a little bit.

1cfe482

stubbing.

2866b37

stubbing.

9956709

some fmt, some head scratching on errors.

531fc43

priming for buckets conditionals.

668f5a9

started scratching out if-unmodified-since.

78d110c

quickcheck instead for some tests.

53daebc

included none and un versions, more tests.

c220540

Merge branch 'master' into dev-MANTA-4341

078f6ee

create and delete precondition. tests to follow.

4422467

include times in failure messages.

84467b2

trying out conditional headers as a separate property.

d53e634

a little tweaking here and there.

1f52262

trying to firm up date formats.

f336485

trying to simplify errors, which isn't going too well.

1029d63

moving to precondition::request just cutting straight to the chase an…

0526d28

…d returning a Value. feels a little better.

a little cleanup.

e09fc30

strings in match checks, moving to regex for string splitting.

5ea2b16

thinking about options.

791180b

make check clean.

d7b693b

trying out a firmer precondition type.

a539711

chudley added 9 commits March 10, 2020 08:17

cleanup.

878aa7e

renaming things, test suite spruce.

af8caeb

more renaming.

4b6990b

etag printing.

fe9fbb8

some cleanup.

271f8cc

Merge remote-tracking branch 'joyent/master' into dev-MANTA-4341

f184f7c

clippy happy.

7465390

shuffle, expose object_not_found().

1853466

trying out a default Conditions.

7a10ffe

chudley requested review from kellymclaughlin and bowrocker March 19, 2020 08:40

kellymclaughlin reviewed Mar 20, 2020

View reviewed changes

chudley added 2 commits March 23, 2020 14:19

review feedback: specialised error, Conditions methods, and trying ou…

fe7e575

…t something for the 'double get' path.

review feedback: having a think about BucketsMdapiError.

1ceb15b

kellymclaughlin suggested changes Mar 26, 2020

View reviewed changes

chudley added 2 commits March 27, 2020 15:54

review feedback: move get_sql.

60fc67d

review feedback: another pass at the 'double get' thing.

17b9772

Merge branch 'master' into dev-MANTA-4341

2484243

kellymclaughlin approved these changes Apr 3, 2020

View reviewed changes

chudley merged commit 548b0f2 into TritonDataCenter:master Apr 6, 2020

chudley deleted the dev-MANTA-4341 branch April 6, 2020 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MANTA-4341 Optimize conditional object operations #47

MANTA-4341 Optimize conditional object operations #47

chudley commented Mar 12, 2020 •

edited

Loading

kellymclaughlin left a comment

kellymclaughlin Mar 19, 2020

chudley Mar 25, 2020

kellymclaughlin Mar 20, 2020

chudley Mar 25, 2020

kellymclaughlin Mar 20, 2020

chudley Mar 25, 2020

kellymclaughlin Mar 20, 2020

chudley Mar 25, 2020

kellymclaughlin Mar 25, 2020

chudley Mar 27, 2020

chudley Mar 30, 2020

kellymclaughlin Mar 31, 2020

kellymclaughlin commented Mar 31, 2020

chudley commented Apr 2, 2020

MANTA-4341 Optimize conditional object operations #47

MANTA-4341 Optimize conditional object operations #47

Conversation

chudley commented Mar 12, 2020 • edited Loading

kellymclaughlin left a 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

kellymclaughlin commented Mar 31, 2020

chudley commented Apr 2, 2020

chudley commented Mar 12, 2020 •

edited

Loading