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

shard-store doesn't support creating entities with specific id$ #11

Open
paolochiodi opened this issue Nov 11, 2015 · 11 comments
Open

shard-store doesn't support creating entities with specific id$ #11

paolochiodi opened this issue Nov 11, 2015 · 11 comments
Labels

Comments

@paolochiodi
Copy link
Contributor

On the save method specs outline three different behaviours:

  • if id is provided, an existing entity should be updated
  • if id isn't provided, a new entity should be created with an auto-generated id
  • if id isn't provided but id$ it, a new entity should be created with that specific id

Currently shard-store seems to "try to support" (see here https://github.com/senecajs/seneca-shard-store/blob/master/index.js#L49) but the underlying "sharder" doesn't support it and this will result undefined https://github.com/senecajs/seneca-shard-store/blob/master/index.js#L66

We can solve this by:

  • adding support to "sharder" for ids generated outside it (keeps compatibility with older versions)
  • remove sharder and use a custom solution that support auto-generated ids (breaks compatibility with older versions because it will distribute entities among shards differently)
  • do not support id$ on the shard store (also breaks current version of seneca-store-tests that use id$ to properly setup the store before running tests)
@paolochiodi
Copy link
Contributor Author

@geek how do you want to proceed on this?

@rjrodger
Copy link
Member

we need to update the sharder to support the spec - sharding should be transparent to users :)

@paolochiodi
Copy link
Contributor Author

What are the secondary goals? I can think of two, but they add up quite a lot of complexity

  • backward database compatibility with current implementation
  • easy scalability (ability to add shards later)

@mcollina
Copy link
Contributor

@rjrodger the whole idea of this store is that ids encode the shard in which they are being created. This is the logic for which this module was built. So, no externally supported ids.

In order to support external ids, we'd need to look things up on all shards, there is little room around that. The other option (for given ids), we'd need to hash the ids and map them using the hash. But that is not as easy as it sounds, as how do we support adding new shards?

Currently sharder supports up to 256 shards (1 byte) without resharding/regenerating all ids.

@paolochiodi
Copy link
Contributor Author

@mcollina you perfectly said what I was thinking.
Supporting full spec will only come with other drawbacks

@rjrodger
Copy link
Member

ah yes good points guys - sharder of course requires control of the ids - so cannot support full spec - but that's ok - it's the tradeoff for sharding

I guess we need a way to parameterise the standard store tests to handle this case?

@mirceaalexandru
Copy link

Yes, we might think on that, is a very good idea to parameterise the
standard store. Now I am changing riak store and there I have some
functions from store that cannot be implemented - like delete all$:true,
list and some more so this will be very good for other stores also.

On Mon, Nov 16, 2015 at 7:23 PM, Richard Rodger [email protected]
wrote:

ah yes good points guys - sharder of course requires control of the ids -
so cannot support full spec - but that's ok - it's the tradeoff for sharding

I guess we need a way to parameterise the standard store tests to handle
this case?


Reply to this email directly or view it on GitHub
#11 (comment)
.

malex

@paolochiodi
Copy link
Contributor Author

I wouldn't parameterise too much: the point of having a standard test suite is to make the stores uniform.

Going a bit OT here, but I think we should only have stores that support all the specs (or a specific subset of it).
Any database/persistence layer that makes difficult or impossibile to do that signals that storing entities may not be the best use case for such a database and it should be used directly through the driver (or a different abstraction). I'm thinking of Riak, Cassandra, ldap...

@mcollina
Copy link
Contributor

The whole point of this is to use the standard store with sharding. Apart from the ids, this should be compliant to the spec.

I'm ok for dropping this one, but I think it's used...

@paolochiodi
Copy link
Contributor Author

@mcollina I think we can make an exception for the shard store (given his use case), but I think we shouldn't make exceptions for every store

@mirceaalexandru
Copy link

@paolochiodi I think that we should have stores for as much DB engines as possible.

On the other hand for those that do not match exactly the standard store tests because the DB do not allow some functions we should specify this in the Readme. This will make difficult to switch between DB engines or use something like shard, but we should allow them to use Riak store (an example) if they need that.

Imagine that you need to use Riak in your project, then you will need to write that plugin by yourself or go directlyto the driver when you can have the Seneca Store interface for what you can use on the DB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants