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

Fix filter cache setting to allow percentages #20335

Merged
merged 1 commit into from Sep 7, 2016

Conversation

Projects
None yet
4 participants
@colings86
Copy link
Member

commented Sep 6, 2016

During adding the new settings infrastructure the option to specify the
size of the filter cache as a percentage of the heap size which accidentally
removed. This change adds that ability back.

In addition the Setting class had multiple .byteSizeSetting methods
which all except one used ByteSizeValue.parseBytesSizeValue to parse
the value. One method used MemorySizeValue.parseBytesSizeValueOrHeapRatio.
This was confusing as the way the value was parsed depended on how many
arguments were provided.

This change makes all Setting.byteSizeSetting methods parse the value
the same way using ByteSizeValue.parseBytesSizeValue and adds
Setting.memorySizeSetting methods to parse settings that express memory
sizes (i.e. can be absolute bytes values or percentages). Relevant settings
have been moved to use these new methods.

Closes #20330

@colings86

View changes

core/src/main/java/org/elasticsearch/repositories/fs/FsRepository.java Outdated
public static final Setting<ByteSizeValue> REPOSITORIES_CHUNK_SIZE_SETTING =
Setting.byteSizeSetting("repositories.fs.chunk_size", "-1", Property.NodeScope);
Setting.memorySizeSetting("repositories.fs.chunk_size", "-1", Property.NodeScope);

This comment has been minimized.

Copy link
@colings86

colings86 Sep 6, 2016

Author Member

Should these two settings have the ability to specify as a percentage of heap as well as absolute bytes?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Sep 6, 2016

Member

I don't think so, I think these should be bytes or size-value only.

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 6, 2016

Contributor

++ to keep byteSizeSetting here

@jasontedor

View changes

core/src/main/java/org/elasticsearch/common/settings/Setting.java Outdated
// percentage, Property... properties) {
// return new Setting<>(key, (s) -> percentage, (s) ->
// ByteSizeValue.parseBytesSizeValue(s, key), properties);
// }

This comment has been minimized.

Copy link
@jasontedor

jasontedor Sep 6, 2016

Member

Can we remove the commented out code?

This comment has been minimized.

Copy link
@colings86

colings86 Sep 6, 2016

Author Member

Oops, I thought I had :)

@jasontedor

View changes

core/src/main/java/org/elasticsearch/indices/IndexingMemoryController.java Outdated
@@ -52,7 +52,8 @@
public class IndexingMemoryController extends AbstractComponent implements IndexingOperationListener, Closeable {

/** How much heap (% or bytes) we will share across all actively indexing shards on this node (default: 10%). */
public static final Setting<ByteSizeValue> INDEX_BUFFER_SIZE_SETTING = Setting.byteSizeSetting("indices.memory.index_buffer_size", "10%", Property.NodeScope);
public static final Setting<ByteSizeValue> INDEX_BUFFER_SIZE_SETTING = Setting.memorySizeSetting("indices.memory.index_buffer_size",
"10%", Property.NodeScope);

This comment has been minimized.

Copy link
@jasontedor

jasontedor Sep 6, 2016

Member

Nit: Can we reformat this by just moving everything after the equals sign to the next line with continuation indentation so that all the parameters and the Setting.memorySizeSetting invocation should fit on one line then?

@colings86 colings86 force-pushed the colings86:fix/20330 branch Sep 6, 2016

@jasontedor

View changes

core/src/main/java/org/elasticsearch/indices/IndicesQueryCache.java Outdated
@@ -49,7 +49,7 @@

public class IndicesQueryCache extends AbstractComponent implements QueryCache, Closeable {

public static final Setting<ByteSizeValue> INDICES_CACHE_QUERY_SIZE_SETTING = Setting.byteSizeSetting(
public static final Setting<ByteSizeValue> INDICES_CACHE_QUERY_SIZE_SETTING = Setting.memorySizeSetting(

This comment has been minimized.

Copy link
@jasontedor

jasontedor Sep 6, 2016

Member

Same thing here, move the invocation to the next line with continuation indentation so that the invocation and its parameters are together?

@jasontedor

This comment has been minimized.

Copy link
Member

commented Sep 6, 2016

@colings86 I left one comment about some commented out code, some nits about formatting, and a response to your question about the chunk size settings.

@s1monw

View changes

core/src/main/java/org/elasticsearch/common/settings/Setting.java Outdated
@@ -591,6 +587,18 @@ public static ByteSizeValue parseByteSize(String s, ByteSizeValue minValue, Byte
return value;
}

public static Setting<ByteSizeValue> memorySizeSetting(String key, ByteSizeValue value, Property... properties) {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 6, 2016

Contributor

can we get some javadocs here and also document what the differences are to the byteSizeSetting methods

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 6, 2016

Contributor

I'd also appreciate some tests here for these methods and also for the settings that are supposed to accept percentages

@@ -0,0 +1,88 @@
/*

This comment has been minimized.

Copy link
@colings86

colings86 Sep 6, 2016

Author Member

Not sure if these tests are better here in one place or if they should be in separate test classes for each class that defines one of the settings?

@colings86

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2016

@s1monw @jasontedor thanks for the reviews, I have pushed a commit that should address your comments

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

LGTM

@colings86 colings86 force-pushed the colings86:fix/20330 branch Sep 7, 2016

Fix filter cache setting to allow percentages
During adding the new settings infrastructure the option to specify the
size of the filter cache as a percentage of the heap size which accidentally
removed. This change adds that ability back.

In addition the `Setting` class had multiple `.byteSizeSetting` methods
which all except one used `ByteSizeValue.parseBytesSizeValue` to parse
the value. One method used `MemorySizeValue.parseBytesSizeValueOrHeapRatio`.
This was confusing as the way the value was parsed depended on how many
arguments were provided.

This change makes all `Setting.byteSizeSetting` methods parse the value
the same way using `ByteSizeValue.parseBytesSizeValue` and adds
`Setting.memorySizeSetting` methods to parse settings that express memory
sizes (i.e. can be absolute bytes values or percentages). Relevant settings
have been moved to use these new methods.

Closes #20330

@colings86 colings86 force-pushed the colings86:fix/20330 branch to 55d9e99 Sep 7, 2016

@colings86 colings86 merged commit 55d9e99 into elastic:master Sep 7, 2016

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author has signed the CLA
Details

@colings86 colings86 deleted the colings86:fix/20330 branch Sep 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.