Skip to content
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

feat: add Features + datastore scoping #188

Merged
merged 1 commit into from
Jun 25, 2022
Merged

feat: add Features + datastore scoping #188

merged 1 commit into from
Jun 25, 2022

Conversation

guseggert
Copy link
Contributor

@guseggert guseggert commented Mar 23, 2022

This is ripped out of https://github.com/guseggert/go-ds-grpc, as I realized it solves a more general problem.

Commit

The motivation for this is to enable "dispatching" datastores that
dynamically implement the type of the datastore they are dispatching
to, so that type assertions behave equivalently on the dispatcher
as on the dispatchee. We also want this to be backwards-compatible
with existing code using type assertions.

At a high level, this works by generating a concrete implementation of
every possible combination of "features", and then picking the right
implementation at runtime. This is necessary due to language
constraints in Go--it is currently impossible to create a concrete
type dynamically with reflection that implements an interface.

"Features" are introduced here, which are supplemental, optional
interfaces that datastores may implement. These are
backwards-compatible with existing "features", which are:

  • Batching
  • CheckedDatastore
  • GCDatastore
  • PersistentDatastore
  • ScrubbedDatastore
  • TTLDatastore
  • TxnDatastore

New features can also be added in a backwards-compatible way. E.g. if
datastore A is scoped down to datastore B, a new feature F is added, and
then implemented on B, then A will continue to implement the same set
of features since it hasn't implemented F yet (and vice versa if F is
implemented on A but not B).

Examples of things this enables:

  • Allow us to deprecate ErrBatchUnsupported (maybe...depending on mount datastores)
  • Allow existing dispatching datastores to support all
    features (keytransform, retrystore, MutexDatastore, autobatch, etc.)
  • Features supported by a Mount datastore could be scoped down to the
    intersection of all children
  • Communication with data about what functionality a datastore
    supports (e.g. for cross-language/RPC support)

Some related issues:

@guseggert guseggert requested a review from Stebalien March 23, 2022 20:18
@guseggert guseggert added this to the Best Effort Track milestone Mar 24, 2022
@guseggert guseggert added the kind/enhancement A net-new feature or improvement to an existing feature label Mar 24, 2022
@guseggert guseggert self-assigned this Mar 24, 2022
@guseggert guseggert force-pushed the feat/scoped branch 5 times, most recently from 8805340 to ed2042e Compare March 26, 2022 18:48
@guseggert guseggert requested review from aschmahmann and removed request for Stebalien March 26, 2022 18:49
@guseggert guseggert marked this pull request as ready for review March 26, 2022 18:50
@guseggert
Copy link
Contributor Author

@aschmahmann pointed out yesterday that we could also use this, along with a new method on mount datastores to return a scoped datastore for a given key/key prefix/query, which effectively solves the problem with mixing multiple datastore types together with a mount datastore.

@guseggert guseggert force-pushed the feat/scoped branch 8 times, most recently from f1aa84f to 8d7bb60 Compare June 13, 2022 21:17
@Jorropo Jorropo self-requested a review June 14, 2022 16:40
@BigLep BigLep removed the request for review from aschmahmann June 14, 2022 16:40
features.go Outdated Show resolved Hide resolved
features.go Outdated Show resolved Hide resolved
features_test.go Outdated Show resolved Hide resolved
null_ds.go Show resolved Hide resolved
scoped/scoped.go Outdated Show resolved Hide resolved
scoped/scoped.go Outdated Show resolved Hide resolved
return featureSet
}

// Wrap returns a datastore based on the source, whose concrete type is scoped down to only the features supported by the target.
Copy link

@Jorropo Jorropo Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is wrong, as far as I can tell, this actually return the intersection of features.

I think the intersection is less useful, and I would like it to return an error instead of trying to silently continue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The elements of set A that are also in set B is the definition of set intersection, the two statements are equivalent.

The reason this doesn't return an error if B is not a subset of A is to allow for both datastores to evolve independently, without breaking consumers of this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this sentence assume that set A always is >= than set B. It's not really important.

scoped/impls.go Outdated Show resolved Hide resolved
@Jorropo
Copy link

Jorropo commented Jun 18, 2022

Minor changes should be needed.

I don't like routines that try to make up with degraded state silently.

If something is not happening as it should, fail loudly, so I (the programmer) can learn about it and fix it.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
guseggert Gus Eggert
The motivation for this is to enable "dispatching" datastores that
dynamically implement the type of the datastore they are dispatching
to, so that type assertions behave equivalently on the dispatcher as
on the dispatchee. We also want this to be backwards-compatible with
existing code using type assertions.

At a high level, this works by generating a concrete implementation of
every possible combination of "features", and then picking the right
implementation at runtime. This is necessary due to language
constraints in Go--it is currently impossible to create a concrete
type dynamically with reflection that implements an interface.

"Features" are introduced here, which are supplemental, optional
interfaces that datastores may implement. These are
backwards-compatible with existing "features", which are:

* Batching
* CheckedDatastore
* GCDatastore
* PersistentDatastore
* ScrubbedDatastore
* TTLDatastore
* TxnDatastore

New features can also be added in a backwards-compatible way. E.g. if
datastore A is scoped down to datastore B, a new feature F is added,
and then implemented on B, then A will continue to implement the same
set of features since it hasn't implemented F yet (and vice versa if F
is implemented on A but not B).

Examples of things this enables:

* Allow us to deprecate ErrBatchUnsupported
* Allow existing dispatching datastores to support all
features (keytransform, retrystore, MutexDatastore, autobatch, etc.)
* Features supported by a Mount datastore could be scoped down to the
intersection of all children
* Communication with data about what functionality a datastore
supports (e.g. for cross-language/RPC support)

Some related issues:

* #160
* #88
Copy link

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return featureSet
}

// Wrap returns a datastore based on the source, whose concrete type is scoped down to only the features supported by the target.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this sentence assume that set A always is >= than set B. It's not really important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants