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

Elasticsearch 6.X and 5.x implementation #51

Merged
merged 28 commits into from
Aug 8, 2017
Merged

Elasticsearch 6.X and 5.x implementation #51

merged 28 commits into from
Aug 8, 2017

Conversation

nanov
Copy link
Contributor

@nanov nanov commented Jul 21, 2017

Elasticsearch 5.x db implementation, base operations. Only native Query DSL queries tested ( with query option native ).

@adrai
Copy link
Contributor

adrai commented Jul 21, 2017

Is it possible to save the version in the data i.e. _version property (like couchdb implementation) so no change on ViewModel is needed...
but only if possible

@adrai
Copy link
Contributor

adrai commented Jul 21, 2017

PS. do you think we could add tests for elasticsearch? and then add it to travis too? https://docs.travis-ci.com/user/database-setup/#ElasticSearch

@adrai
Copy link
Contributor

adrai commented Jul 21, 2017

And may you describe (in the readme) what is the difference between elasticsearch and elasticsearch5 implementation?

@nanov
Copy link
Contributor Author

nanov commented Jul 21, 2017

Elasticsearch gives some error with the "_version" property, one option is to name it other way but I think it is better to use the native Elasticsearch versioning.

This implementation will work also without modifying ViewModel ( as i add the version manually to the ViewModel on get and search ).

Tests are definitely a good idea and planned, hopefully I will get some time over the weekend to implement those.

@nanov
Copy link
Contributor Author

nanov commented Jul 21, 2017

PS. you are incredibly fast ;)

@adrai
Copy link
Contributor

adrai commented Jul 21, 2017

not always 😜

@nanov
Copy link
Contributor Author

nanov commented Jul 26, 2017

Well I moved on adapting the whole thing for Elasticsearch 6.X, the biggest diffrence is that from now on Elasticsearch 6 allows only one type per index ( eventually types are going to be removed in version 7 ). I've cleaned up the whole thing ( althoguht there is a lot more to be done ) and tested it on our working system for our purposes.

@nanov
Copy link
Contributor Author

nanov commented Jul 28, 2017

Now I am having the next dilemma, it would be good to have some specific Elasticsearch properties inside the repository definition ( like the indexes for mongoDb ), as the whole metadata is not stored inside the repository some changes will need to be made.

One solution would be to store the given metadata as a whole, this way each implementation could use the appropriate properties from the metadata when needed.

Another solution would be to take just a field named as the implementation and store it, for example if i use elasticsearch, if a field named "elasticsearch" is present in the viewmodel metadata ( ie. in the collection definition in node-event-denormalizer ) this field will be stored in the repository and be present in the implementation for use.

This is needed as there are some Elasticsearch index and alias settings which are really dependent on the nature and size of data you are storing.

What do you think would be a good approach on this?

@adrai
Copy link
Contributor

adrai commented Jul 29, 2017

Perhaps, extend the commit method on vm and on repository to accept an optional additional argument...

@nanov
Copy link
Contributor Author

nanov commented Jul 31, 2017

Well the thing is that this would ideally be in the collection definition ( exactly like the indexes property ), because it would affect the way the mapping for the index would be created. I guess this is more denormalizer related than viewmodel, maybe we should continue the discussion there?

@adrai
Copy link
Contributor

adrai commented Aug 1, 2017

If it works to have it in the checkConnection function it's better 😉

@nanov
Copy link
Contributor Author

nanov commented Aug 1, 2017

It will be eventually used in the checkConnection method, the question is how the user is going to pass the desired definition ?

The definition should be on the collection level for example :

module.exports = cqrsEventDenormalizer.defineCollection({
	name: 'coolEsIndexedCollection',
	context: 'coolDomain',
	defaultPayload: 'payload',
        esSettings:{ 
           settings: { // will be merged with default settings
		number_of_shards: 1, // default
		number_of_replicas: 0 // default
	    },
            mappings : { // will be merged with default (dynamic )mappings in the corresponding type
               properties: { 
                  title:    { "type": "text"  }, 
                  name:     { "type": "text"  }, 
                  age:      { "type": "integer" }, 
               }
            }
        }
});

@nanov
Copy link
Contributor Author

nanov commented Aug 1, 2017

Is there any particular reason why not to store and pass the meta from the collection to the repository?

@adrai
Copy link
Contributor

adrai commented Aug 1, 2017

No explict reason.

@nanov
Copy link
Contributor Author

nanov commented Aug 3, 2017

Together with thenativeweb/node-cqrs-eventdenormalizer#56 a full elasticsearch 6.X ( tested with 5.X as well ) is available now. With the right settings the indexing and mapping can be tweaked per repository ( or global ) basis for maximum performance.

@adrai
Copy link
Contributor

adrai commented Aug 3, 2017

As soon as you give the "ok" and add some tests, I will merge both PRs.

@nanov
Copy link
Contributor Author

nanov commented Aug 4, 2017

I think the first phase is complete. It could definitely use some tweaks and optimizations but those will be done in the next iteration after some more serious in-production use.

@nanov
Copy link
Contributor Author

nanov commented Aug 4, 2017

Another thing, when you merge, could you give the credit combined also to eCollect, as the PRs are done on company time?

@adrai
Copy link
Contributor

adrai commented Aug 4, 2017

release notes (+tweet) ok?

@nanov
Copy link
Contributor Author

nanov commented Aug 4, 2017

More than perfect!

@nanov
Copy link
Contributor Author

nanov commented Aug 7, 2017

I think both PRs are ready for merge.

@adrai
Copy link
Contributor

adrai commented Aug 7, 2017

ok... let's wait 24h in case you find something else, ok?

@adrai
Copy link
Contributor

adrai commented Aug 7, 2017

by the way... why is it named elasticsearch6 and not 5?

@nanov
Copy link
Contributor Author

nanov commented Aug 7, 2017

For the wait, sure!

For the version :

TL:DR
Basically it turned out to be a 6.x implementation which is compatible with 5.x and not the opposite.

Explanation
Well I have started targeting elasticsearch 5.x, but over the process, with stable elasticsearch 6.x over the corner ( that we already use on one of our environments ), i have decided to make it compatible with both ( biggest change is the removal of mapping types ).

@nanov nanov changed the title Elasticsearch 5.x implementation Elasticsearch 6.X and 5.x implementation Aug 8, 2017
@@ -290,9 +290,11 @@ _.extend(Mongo.prototype, {
ensureIndexes: function() {
var self = this;

if (!this.isConnected || !this.collectionName || !this.indexes) return;
var indexes = (this.repositorySettings && this.repositorySettings.mongodb && this.repositorySettings.indexes) ? this.repositorySettings.indexes : this.indexes
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be:

var indexes = (this.repositorySettings && this.repositorySettings.mongodb && this.repositorySettings.mongodb.indexes) ? this.repositorySettings.mongodb.indexes : this.indexes;

lib/viewmodel.js Outdated
@@ -28,6 +28,10 @@ function ViewModel (attr, repository) {
this.id = _.clone(attr.id);
}

// version addition, used by elasticsearch5, 0 means a new model
Copy link
Contributor

Choose a reason for hiding this comment

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

comment: elasticsearch6 ;-)

@nanov
Copy link
Contributor Author

nanov commented Aug 8, 2017

Absolutely, fixed.

@adrai adrai merged commit a0547c5 into thenativeweb:master Aug 8, 2017
@adrai
Copy link
Contributor

adrai commented Aug 8, 2017

@nanov What is your twitter handle?

@nanov
Copy link
Contributor Author

nanov commented Aug 8, 2017

https://twitter.com/ecollect

Thank you :)

@adrai
Copy link
Contributor

adrai commented Aug 8, 2017

And your personal?

@nanov
Copy link
Contributor Author

nanov commented Aug 8, 2017

I don't really use my twitter, you can just mention the company, it is totally fine.

@adrai
Copy link
Contributor

adrai commented Aug 8, 2017

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

Successfully merging this pull request may close these issues.

2 participants