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.

Sign up for GitHub

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

Jump to bottom

Expose store.type table parameter and node.store.allow_mmap setting. #9125

Merged
merged 6 commits into from
Sep 10, 2019
Merged

Expose store.type table parameter and node.store.allow_mmap setting. #9125

merged 6 commits into from
Sep 10, 2019

Conversation

kovrus
Copy link
Contributor

@kovrus kovrus commented Sep 6, 2019 •
edited
Loading

Summary of the changes / Why this improves CrateDB

See commits.

Checklist

  • User relevant changes are recorded in CHANGES.txt
  • Touched code is covered by tests
  • Documentation has been updated if necessary
  • CLA is signed
  • This does not contain breaking changes, or if it does:
    • It is released within a major release
    • It is recorded in CHANGES.txt
    • It was marked as deprecated in an earlier release if possible
    • You've thought about the consequences and other components are adapted
      (E.g. AdminUI)

@kovrus kovrus requested review from seut and mfussenegger September 6, 2019 14:01
@seut
Copy link
Member

seut commented Sep 6, 2019

I think we could directly include related fixes/changes like elastic/elasticsearch#38940 inside the PR as well.

@mfussenegger
Copy link
Member

Should we also document this new type and add a release notes entry?

@kovrus
Copy link
Contributor Author

kovrus commented Sep 9, 2019

@seut backported elastic/elasticsearch#38940 and a couple of refactoring commits. If it is ok, I'd like to get this PR merged and follow up with renaming the allow_mmapfs setting, documenting the new store type and adding release notes in the separate PR.

@kovrus kovrus changed the title Add hybridfs store type. Expose store.type table parameter and node.store.allow_mmap setting. Sep 9, 2019
@kovrus kovrus force-pushed the r/hybridfs branch 2 times, most recently from 76d1324 to b7fe596 Compare September 9, 2019 14:53
kovrus
kovrus commented Sep 9, 2019
View reviewed changes
es/es-server/src/main/java/org/elasticsearch/index/IndexModule.java

public static final Setting<String> INDEX_STORE_TYPE_SETTING =
new Setting<>("index.store.type", "", Function.identity(), Property.IndexScope, Property.NodeScope);
new Setting<>("index.store.type", "fs", Function.identity(), Property.IndexScope, Property.NodeScope);
Copy link
Contributor Author

@kovrus kovrus Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

set the fs value explicitly as the setting's default value.

@kovrus kovrus requested review from mkleen and removed request for mfussenegger September 9, 2019 15:01
seut
seut reviewed Sep 9, 2019
View reviewed changes
Copy link
Member

@seut seut left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Cool stuff, I've added just one comment about the fs documentation.
Also I think we should add a CHANGES entry about that we've changed the default FS to hybridfs.

blackbox/docs/sql/statements/create-table.rst Outdated Show resolved Hide resolved
@kovrus kovrus requested a review from seut September 9, 2019 15:57
seut
seut approved these changes Sep 10, 2019
View reviewed changes
Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

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

👍 Lgtm

@kovrus kovrus added the ready-to-merge Let Mergify merge the PR once approved and checks pass label Sep 10, 2019
@mergify mergify bot merged commit 05fe1ef into master Sep 10, 2019
@mergify mergify bot deleted the r/hybridfs branch September 10, 2019 08:26
@kovrus kovrus mentioned this pull request Sep 13, 2019
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seut seut seut approved these changes

@mkleen mkleen Awaiting requested review from mkleen

Assignees
No one assigned
Labels
ready-to-merge Let Mergify merge the PR once approved and checks pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kovrus @seut @mfussenegger