search

loopbackio / loopback-connector-elastic-search

Strongloop Loopback connector for Elasticsearch
MIT License
78 stars 56 forks source link

"refresh: true" for change operations #77

Closed wolfgang-s closed 7 years ago

wolfgang-s commented 7 years ago

all tests pass now in 8 seconds instead of 2 minutes.

remove comments as you like ...

only test which has still a timeout is "should only delete instances that satisfy the where condition"

here i was not able to remove it? No idea why ... very weird, has nothing to do with es or refresh?!

slnode commented 7 years ago

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

aquid commented 7 years ago

@wolfgang-s Thanks for the PR. Please find my comments inline and update the PR with your previous solution. We are very soon to release a new version with filter bug fixes and it would be a great help if you can submit the PR asap.

wolfgang-s commented 7 years ago

@aquid I don't see any comments? This is my first PR maybe I'm missing something out? :)

What do you mean with "previous" solution, do you mean this solution in this PR?

The solution in my master branch makes this refresh settings configurable, which is bad I think, because all other datasource connectors will make all DB changes available instantly (right?) ...

wolfgang-s commented 7 years ago

Interesting read: https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-refresh.html

What do you think? Should we make it configurable by model? Defaulting to "refresh: true"?

I personally think, putting load onto the database is not the problem, but slow response times in the frontend is.

Another idea would be to make it configurable per model AND per call. So let's say, when I insert articles via a CSV uploader, I do not want to index every time for every article. But when I create a single article, I do want it to be visible instantly. <- Problem here, I have no clue how to realize this.

I think my solution for now is pretty neat. I could submit another PR later ...

aquid commented 7 years ago

@wolfgang-s

I don't see any comments?

Look here https://github.com/strongloop-community/loopback-connector-elastic-search/pull/77#discussion_r102559822

What do you mean with "previous" solution, do you mean this solution in this PR?

No I meant https://github.com/wolfgang-s/loopback-connector-elastic-search/commit/26f8aeeabc6cbfbe712d635db2c2b5cc4b0e9831

because all other datasource connectors will make all DB changes available instantly (right?)

Yes, most of the connectors will do that, but as we know that making refresh:true by default can create huge performance issues in elasticsearch, so I would rather leave this decision to the user and make this option configurable.

Another idea would be to make it configurable per model AND per call

This would be a perfect solution. You can have a look at this issue and validate if this could be useful for doing per request refresh option.

Let me know if you would be interested in updating the PR with making the option configurable.

wolfgang-s commented 7 years ago

@aquid Ok, I make it configurable as in my master branch and default true (!?), per model and per call using the options parameter.

I still don't see any inline comments :(

aquid commented 7 years ago

@wolfgang-s

I make it configurable as in my master branch and default true (!?)

Default to true is perfect.

aquid commented 7 years ago

@wolfgang-s Also don't change the existing tests instead, create a new file/tests without timeout and change the existing ones as pending/inactive ( change describe as xdescribe) that way anyone running the tests with refresh:false would still be able to run test successfully by manually editing the describe block.

wolfgang-s commented 7 years ago

@aquid everything should be fine now ;)

what do you think?

aquid commented 7 years ago

@wolfgang-s Almost there. just few more changes.

  1. As you can see there's a merge conflict now, please rebase your changes over latest develop branch and re-submit your PR.
  2. I want the array to come from datasource file. added comment to explain that and this time you should be able to see it.
  3. Let's not club options and refresh feature both in a single PR. so please make this one specific to refresh and you can send a new PR for options

FYI - options added looks great but we need to be sure that this supports earlier versions too where options were not passed. moreover, I see this code from mongo-connector which makes me think that we should also be implementing this.

wolfgang-s commented 7 years ago

@aquid

  1. I will create 2 PRs (new ones) now based on the develop branch.
  2. Array will come from datasource file with fallback.
  3. This "options" fallback from the mongo-connector is already in there (for destroyAll) ... And for the other functions in mongo-connector I don't see it there ... I will leave it as is, this change was merged already in 2015, so I guess nobody will have such an old version of the db connector ...
wolfgang-s commented 7 years ago

Here you go: https://github.com/strongloop-community/loopback-connector-elastic-search/pull/81 This pull request can be ignored now.

pulkitsinghal commented 7 years ago

To me it seems refreshOn is not per model but rather applies to all models, am I wrong? If that work is remaining, then should we create a new issue for the future?