Skip to content

Conversation

@s1monw
Copy link
Contributor

@s1monw s1monw commented Dec 4, 2015

This commit restores the chunk size of 512kb lost in a previous but unreleased
refactoring. At the same time it removes the configurability of:

  • indices.recovery.file_chunk_size - now fixed to 512kb
  • indices.recovery.translog_ops - removed without replacement
  • indices.recovery.translog_size - now fixed to 512kb
  • indices.recovery.compress - file chunks are not compressed due to lucene's compression but translog operations are.

The compress option is gone entirely and compression is used where it makes sense. On sending files of the index
we don't compress as we rely on the lucene compression for stored fields etc.

Relates to #15161

@s1monw s1monw added >breaking review :Core/Infra/Settings Settings infrastructure and APIs :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v5.0.0-alpha1 labels Dec 4, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

512 * 1024 👅

Copy link
Contributor

Choose a reason for hiding this comment

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

new ByteSizeValue("512kb").bytes() :trollface:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fucking returns a stupid long I don't wanna cast

Copy link
Contributor

Choose a reason for hiding this comment

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

bytesAsInt()

@bleskes
Copy link
Contributor

bleskes commented Dec 4, 2015

Production code LGTM. I left some concerns about the testing change.

This commit restores the chunk size of 512kb lost in a previous but unreleased
refactoring. At the same time it removes the configurability of:
 * `indices.recovery.file_chunk_size` - now fixed to 512kb
 * `indices.recovery.translog_ops` - removed without replacement
 * `indices.recovery.translog_size` - now fixed to 512kb
 * `indices.recovery.compress` - file chunks are not compressed due to lucene's compression but translog operations are.

The compress option is gone entirely and compression is used where it makes sense. On sending files of the index
we don't compress as we rely on the lucene compression for stored fields etc.

Relates to elastic#15161
@s1monw s1monw force-pushed the remove_recovery_settings branch from 09bd792 to 37b60bd Compare December 8, 2015 10:22
@s1monw
Copy link
Contributor Author

s1monw commented Dec 8, 2015

@bleskes I had to add one more commit - can you take a quick look?

@bleskes
Copy link
Contributor

bleskes commented Dec 8, 2015

LGTM

@s1monw
Copy link
Contributor Author

s1monw commented Dec 11, 2015

@bleskes I refined this change one more time to allow tests to override the chunksize which I think is a good tool to have (pkg private setter) and I also fixed the stream closing issue. I think it's ready now. Do you wanna take one more look?

Copy link
Contributor

Choose a reason for hiding this comment

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

add comment about what this is not final (i.e., for testing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the setter has a comment

@bleskes
Copy link
Contributor

bleskes commented Dec 11, 2015

LGTM (better than before). left some minor optional suggestions.

s1monw added a commit that referenced this pull request Dec 11, 2015
Restore chunksize of 512kb on recovery and remove configurability
@s1monw s1monw merged commit 90ff1ad into elastic:master Dec 11, 2015
@s1monw s1monw deleted the remove_recovery_settings branch December 11, 2015 14:27
@clintongormley clintongormley removed the :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. label Dec 14, 2015
alamzeeshan added a commit to alamzeeshan/elasticsearch that referenced this pull request Jan 31, 2017
Updated document as per this change : elastic#15235
clintongormley pushed a commit that referenced this pull request Jan 31, 2017
Updated document as per this change : #15235
clintongormley pushed a commit that referenced this pull request Jan 31, 2017
Updated document as per this change : #15235
clintongormley pushed a commit that referenced this pull request Jan 31, 2017
Updated document as per this change : #15235
clintongormley pushed a commit that referenced this pull request Jan 31, 2017
Updated document as per this change : #15235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants