Navigation Menu

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

Change default store_type to fs #80

Merged
merged 1 commit into from Jul 3, 2019
Merged

Conversation

ebadyano
Copy link
Contributor

Closes #79

@ebadyano ebadyano requested a review from dliappis June 27, 2019 11:27
@ebadyano
Copy link
Contributor Author

ebadyano commented Jun 27, 2019

@dliappis One concern I have is that @danielmitterdorfer very recently delivered a chance in f1e4143 where he used hybridfs as a default and not fs; wonder if I'm missing something for #79. Also I ran nested with the default changes to fs, seems to work fine. Any ideas?

@dliappis
Copy link
Contributor

where he used hybridfs as a default and not fs

As per https://www.elastic.co/guide/en/elasticsearch/reference/current/index-modules-store.html#file-system:

fs
Default file system implementation. This will pick the best implementation depending on the operating environment, which is currently hybridfs on all supported systems but is subject to change.

So currently (as of ES 7.0, please do double check the versions in the docs) fs equals hybridfs so we are ok.

I'd be careful about backporting this PR though to older branches esp. since fs became available after 7.0.

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM

@ebadyano ebadyano merged commit eb93f4c into elastic:master Jul 3, 2019
@danielmitterdorfer
Copy link
Member

I'd be careful about backporting this PR though to older branches esp. since fs became available after 7.0.

@ebadyano, @dliappis I think it is safe to backport this all the way until 5 as fs is available there as well? However, he store_type track parameter is currently available in 7, 7.2, 7.1 and 7.0 and I think we should backport this PR at least to these branches as well.

@dliappis
Copy link
Contributor

dliappis commented Jul 8, 2019

Thanks @danielmitterdorfer 👍 on backporting to 7, 7.2, 7.1 and 7.0.

ebadyano added a commit that referenced this pull request Jul 9, 2019
ebadyano added a commit that referenced this pull request Jul 9, 2019
ebadyano added a commit that referenced this pull request Jul 9, 2019
@ebadyano
Copy link
Contributor Author

ebadyano commented Jul 9, 2019

Backported to 7, 7.1, 7.0. I don't see 7.2 branch under rally-tracks..

@danielmitterdorfer
Copy link
Member

I don't see 7.2 branch under rally-tracks..

You're right. Thanks for backporting it!

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

Successfully merging this pull request may close these issues.

Use the default store type instead of assuming one
3 participants