docs(rfc): Serialising GMS Updates with Preconditions

[mattmatravers]

Checklist

• The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• Links to related issues (if applicable)

[mattmatravers]

This PR describes many of the points raised in #5635

[jjoyce0510]

Makes sense to me. Thanks for the clear description up to this point!

[jjoyce0510]

per-aspect tagging, this makes sense.

[jjoyce0510]

@RyanHolstien Is currently working on PATCH API support on the GMS ingest path. I'm wondering if we should account for it. I'm assuming that by default PATCH would ignore the version tag

[jjoyce0510]

This is true

[jjoyce0510]

This is the biggest issue with this. I think most companies using DataHub simply wouldn't bother with this, and thus would not benefit from it!

[mattmatravers]

Yeah I re-read this document fully yesterday and I need to remove some ideas from the main section. This is one of them.

[jjoyce0510]

How does the client know what they read is still valid?

[mattmatravers]

I guess they'll never know, but they will know the version of the aspect they just read. The clients then have the option of passing that version back to GMS during an update request.

[jjoyce0510]

If a conflict occurs, what should the client do? Just abort or log a warning or something, presumably?

[mattmatravers]

The client will have to manage the retry operation or just fail depending on business requirements. GMS won't have enough context to manage this without a suitable patching language that @RyanHolstien is working on.

[jjoyce0510]

@RyanHolstien Please have a look here.

With Ryan's changes, we'll be able to support a specific set of PATCH formats, but it may not always suffice for addressing the above use case (if we don't formally support a patch of the given type. Ryan can elaborate.

[RyanHolstien]

As long as the path drills down far enough, JsonPatch semantics are capable of covering this scenario. Basically you can specify precise fields to modify and it will not impact the rest of the aspect, but if you target the top level of the aspect you may be overwriting things. Example:

JsonPatch for Ownership at top level:
op: ADD
path: /
value:
{ owners: { urn: urn:li:corpuser:owner1 : {...} }

vs JsonPatch at lower level:
op: ADD
path: /owners/urn:li:corpuser:owner1
value: {...}

The first one will wipe out other values in the array, but the second one will not, this is why the SDK implementation is very limited in scope since JsonPatch is almost too flexible in what it allows. It can very much accommodate this use case though.

[mattmatravers]

Makes sense, thanks. I'd be keen to know if JsonPatch can handle idempotent updates like "ensure this set contains these values". But I guess clients will need some form of serialisation guarantees regardless of whether it's a PATCH update or a full replacement.

[RyanHolstien]

Those are two different things in my mind:

1. Idempotency of updates -> Guarantees that if you add owner1 multiple times, it doesn't add it repeatedly. This is ensured

2. Verification of particular values -> Guarantees that owner1 is only added if owner2 is present (or something to this effect? Not sure exactly what you mean here). This is not ensured. Maybe you mean:

add owner1 happens at same time as add owner2, owner1 AND owner2 should appear?

This should also be ensured per transactional consistency.

[mattmatravers]

1. That's good news, thanks 👍 .
2. I'm talking about a scenario where one property of an aspect gets updated using another property as a base state. If the base state is changed then that update is no longer valid, so we will need to offer serialisation for such stateful updates (whether PATCH or UPSERT).

[jjoyce0510]

Still has the overwrite / clobbering problem

[jjoyce0510]

Very slow! Also doesn't well support the MCE pathway, as you mentioned.

[jjoyce0510]

Check out the term "etag" -- its used across other DBs to provide a "version tag" to a document. I'm hoping we can simply add something similar to all of our get / put pathways

[mattmatravers]

I like it. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag seems to cover roughly what we need, though we'd need to think about the multiple aspect use case.

[mattmatravers]

Even better article here, which describes almost exactly what we want: https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests#avoiding_the_lost_update_problem_with_optimistic_locking

[jjoyce0510]

We can just use total aspect count, instead of version. Serves the same purpose

[mattmatravers]

As discussed on video chat, this might not work if we limit the retention of older versions. We need something that is deterministic and not subject to race conditions.

[jjoyce0510]

Should be backwards compatible to simply add totalAspect count somewhere

[mattmatravers]

I'm going to re-organise a lot of this document to be clear what designs are viable and which are not.

[jjoyce0510]

The challenge here is that we'd need to write our own operation DSL. This is what MongoDB has done!

[jjoyce0510]

I think it's possible that we do provide an optional version tag in the Kafka ingest path as well, but there is a higher likelihood of out of date updates. Also, it's obviously not possible for a client to perform a retry since they won't know what failed...

[jjoyce0510]

Merging this RFC to keep for history. Thanks for the awesome contrib and hoping to get this implementation in shortly!