Add field data memory circuit breaker #4261

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@dakrone
Member

dakrone commented Nov 26, 2013

This adds the field data circuit breaker, which is used to estimate
the amount of memory required to load field data before loading it. It
then raises a CircuitBreakingException if the limit is exceeded.

It is configured with two parameters:

indices.fielddata.cache.breaker.limit - the maximum number of bytes
of field data to be loaded before circuit breaking. Defaults to
indices.fielddata.cache.size if set, unbounded otherwise.

indices.fielddata.cache.breaker.overhead - a contast for all field
data estimations to be multiplied with before aggregation. Defaults to
1.03.

Both settings can be configured dynamically using the cluster update
settings API.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Nov 26, 2013

Contributor

Cool stuff @dakrone

Contributor

s1monw commented Nov 26, 2013

Cool stuff @dakrone

@@ -60,6 +60,9 @@ By default, `indices` stats are returned. With options for `indices`,
Transport statistics about sent and received bytes in
cluster communication
+`breaker`::

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

I think breaker is too generic a name?

@jpountz

jpountz Dec 6, 2013

Contributor

I think breaker is too generic a name?

This comment has been minimized.

@dakrone

dakrone Dec 6, 2013

Member

How about circuit-breaker?

@dakrone

dakrone Dec 6, 2013

Member

How about circuit-breaker?

@jpountz

View changes

src/main/java/org/elasticsearch/index/fielddata/FieldDataEstimator.java
+ assert term != null;
+ long bytes = term.length;
+ // 64 bytes for 'long' ordinal
+ bytes += 64;

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

do you mean 64 bits instead of bytes?

@jpountz

jpountz Dec 6, 2013

Contributor

do you mean 64 bits instead of bytes?

This comment has been minimized.

@dakrone

dakrone Dec 6, 2013

Member

It actually should just be "64 bytes for miscellaneous overhead", I'll change it.

@dakrone

dakrone Dec 6, 2013

Member

It actually should just be "64 bytes for miscellaneous overhead", I'll change it.

@jpountz

View changes

...a/org/elasticsearch/index/fielddata/plain/PackedArrayIndexFieldData.java
- return new PackedArrayIndexFieldData(index, indexSettings, fieldNames, type, cache, numericType);
+ public IndexFieldData<AtomicNumericFieldData> build(Index index, @IndexSettings Settings indexSettings, FieldMapper.Names fieldNames,
+ FieldDataType type, IndexFieldDataCache cache, CircuitBreakerService breakerService) {
+ // Ignore Circuit Breaker

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

Looks like it is not being ignored? :)

@jpountz

jpountz Dec 6, 2013

Contributor

Looks like it is not being ignored? :)

This comment has been minimized.

@dakrone

dakrone Dec 6, 2013

Member

Hah, yes, I did actually use it, I'll remove that :)

@dakrone

dakrone Dec 6, 2013

Member

Hah, yes, I did actually use it, I'll remove that :)

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Dec 6, 2013

Member

I realized that the FieldDataEstimator class is no longer needed, as estimations have been moved into their respective field data loading classes, so I'll remove it.

Member

dakrone commented Dec 6, 2013

I realized that the FieldDataEstimator class is no longer needed, as estimations have been moved into their respective field data loading classes, so I'll remove it.

@jpountz

View changes

...a/org/elasticsearch/index/fielddata/plain/PackedArrayIndexFieldData.java
+ double estimatedBytes = ((RamAccountingTermsEnum)termsEnum).getTotalBytes();
+ breaker.addWithoutBreaking(-(long)((estimatedBytes * breaker.getOverhead()) - actualUsed));
+ } else {
+ logger.warn("Trying to adjust circuit breaker, but TermsEnum has not been wrapped!");

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

Should we have an assertion here?

@jpountz

jpountz Dec 6, 2013

Contributor

Should we have an assertion here?

This comment has been minimized.

@dakrone

dakrone Dec 6, 2013

Member

I think an assertion makes sense, I'll do that instead of the if statement.

@dakrone

dakrone Dec 6, 2013

Member

I think an assertion makes sense, I'll do that instead of the if statement.

@jpountz

View changes

...a/org/elasticsearch/index/fielddata/plain/PackedArrayIndexFieldData.java
+ @Override
+ public long bytesPerValue(BytesRef term) {
+ // Estimate about 10x compression for numbers, but at least 4 bytes
+ return Math.max(type.requiredBits() / 10, 4);

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

The comment confuses me a bit: Since you are dividing the number of bits per 10 although this method should return bytes, I guess you meant "Estimate about 0.8 (8 / 10) compression ratio for numbers..." (given that a byte is 8 bits)?

@jpountz

jpountz Dec 6, 2013

Contributor

The comment confuses me a bit: Since you are dividing the number of bits per 10 although this method should return bytes, I guess you meant "Estimate about 0.8 (8 / 10) compression ratio for numbers..." (given that a byte is 8 bits)?

This comment has been minimized.

@dakrone

dakrone Dec 6, 2013

Member

Yes, that is a confusing comment, I'll fix that!

@dakrone

dakrone Dec 6, 2013

Member

Yes, that is a confusing comment, I'll fix that!

@jpountz

View changes

...va/org/elasticsearch/index/fielddata/plain/PagedBytesIndexFieldData.java
+ bytes = (long)((double)bytes / 1.5) + 1;
+ return bytes;
+ }
+

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

this looks duplicated from FieldDataEstimator.estimateStringFieldData, could we try to share code with this other method?

@jpountz

jpountz Dec 6, 2013

Contributor

this looks duplicated from FieldDataEstimator.estimateStringFieldData, could we try to share code with this other method?

This comment has been minimized.

@dakrone

dakrone Dec 6, 2013

Member

Yea, FieldDataEstimator is going away entirely.

@dakrone

dakrone Dec 6, 2013

Member

Yea, FieldDataEstimator is going away entirely.

@@ -59,7 +58,7 @@ public IndicesFieldDataCache(Settings settings) {
super(settings);
this.size = componentSettings.get("size", "-1");
this.expire = componentSettings.getAsTime("expire", null);
- computeSizeInBytes();
+ this.sizeInBytes = computeSizeInBytes();

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

+1 I like this approach better!

@jpountz

jpountz Dec 6, 2013

Contributor

+1 I like this approach better!

@jpountz

View changes

src/main/java/org/elasticsearch/search/CircuitBreakerStats.java
+ @Override
+ public void writeTo(StreamOutput out) throws IOException {
+ out.writeLong(maximum);
+ out.writeLong(estimated);

This comment has been minimized.

@jpountz

jpountz Dec 6, 2013

Contributor

maybe writeVLong would be better suited?

@jpountz

jpountz Dec 6, 2013

Contributor

maybe writeVLong would be better suited?

This comment has been minimized.

@dakrone

dakrone Dec 6, 2013

Member

It's possible that the estimation could be negative (in the case of a bug, or bad calculations), so I'd rather stick with writeLong since writeVLong doesn't support writing negative values.

@dakrone

dakrone Dec 6, 2013

Member

It's possible that the estimation could be negative (in the case of a bug, or bad calculations), so I'd rather stick with writeLong since writeVLong doesn't support writing negative values.

This comment has been minimized.

@s1monw

s1monw Dec 8, 2013

Contributor

Maybe you should just pass Math.max(0, estimated)? When can it be negative?

@s1monw

s1monw Dec 8, 2013

Contributor

Maybe you should just pass Math.max(0, estimated)? When can it be negative?

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Dec 11, 2013

Member

Updated code and force-pushed another squashed commit (because there were going to be merge conflicts regardless, and I'd rather rebase and deal with them now rather than after reviews).

Changes:

  • Move all the files into better/more-applicable packages
  • Breaker stats are now under the key fielddata_breaker and the class is called FieldDataBreakerStats
  • Use constants instead of strings for reused field data filter settings
  • TermsEnum is wrapped in a filter if BlockTreeStats can't be used (for people using custom postings formats)
  • Fix "unwinding" of breaker in the event a different exception occurs while loading field data
  • De-interface-ify MemoryAggregatingCircuitBreaker to become concrete MemoryCircuitBreaker
  • Logger passed through to MemoryCircuitBreaker to preserve which area is using the breaker
  • Remove "field data" from strings in MemoryCircuitBreaker to make it a bit more generic (reflecting the package move to common.breaker)

I may have forgotten other changes that went in, so more reviews welcome :)

Member

dakrone commented Dec 11, 2013

Updated code and force-pushed another squashed commit (because there were going to be merge conflicts regardless, and I'd rather rebase and deal with them now rather than after reviews).

Changes:

  • Move all the files into better/more-applicable packages
  • Breaker stats are now under the key fielddata_breaker and the class is called FieldDataBreakerStats
  • Use constants instead of strings for reused field data filter settings
  • TermsEnum is wrapped in a filter if BlockTreeStats can't be used (for people using custom postings formats)
  • Fix "unwinding" of breaker in the event a different exception occurs while loading field data
  • De-interface-ify MemoryAggregatingCircuitBreaker to become concrete MemoryCircuitBreaker
  • Logger passed through to MemoryCircuitBreaker to preserve which area is using the breaker
  • Remove "field data" from strings in MemoryCircuitBreaker to make it a bit more generic (reflecting the package move to common.breaker)

I may have forgotten other changes that went in, so more reviews welcome :)

@imotov

View changes

...asticsearch/indices/fielddata/breaker/InternalCircuitBreakerService.java
+ this.maxBytes = settings.getAsBytesSize(CIRCUIT_BREAKER_MAX_BYTES_SETTING, new ByteSizeValue(fieldDataMax)).bytes();
+ this.overhead = settings.getAsDouble(CIRCUIT_BREAKER_OVERHEAD_SETTING, DEFAULT_OVERHEAD_CONSTANT);
+
+ this.breaker = new MemoryCircuitBreaker(new ByteSizeValue(maxBytes), overhead, 0, logger);

This comment has been minimized.

@imotov

imotov Dec 12, 2013

Member

It looks like breaker is initialized twice. Here and then in doStart()

@imotov

imotov Dec 12, 2013

Member

It looks like breaker is initialized twice. Here and then in doStart()

This comment has been minimized.

@dakrone

dakrone Dec 13, 2013

Member

I'll remove the doStart() one.

@dakrone

dakrone Dec 13, 2013

Member

I'll remove the doStart() one.

@imotov

View changes

...asticsearch/indices/fielddata/breaker/InternalCircuitBreakerService.java
+ super(settings);
+ this.indicesService = indicesService;
+ long fieldDataMax = fieldDataCache.computeSizeInBytes();
+ this.maxBytes = settings.getAsBytesSize(CIRCUIT_BREAKER_MAX_BYTES_SETTING, new ByteSizeValue(fieldDataMax)).bytes();

This comment has been minimized.

@imotov

imotov Dec 13, 2013

Member

It would be nice to support "% of heap size" notation here as well.

@imotov

imotov Dec 13, 2013

Member

It would be nice to support "% of heap size" notation here as well.

This comment has been minimized.

@dakrone

dakrone Dec 13, 2013

Member

Agreed, but how about making that a separate PR after this gets merged? I think it makes the reviews easier so I don't accidentally introduce bugs while trying to add this feature.

@dakrone

dakrone Dec 13, 2013

Member

Agreed, but how about making that a separate PR after this gets merged? I think it makes the reviews easier so I don't accidentally introduce bugs while trying to add this feature.

@@ -89,6 +93,10 @@ public void onUnload(FieldMapper.Names fieldNames, FieldDataType fieldDataType,
evictionsMetric.inc();
}
if (sizeInBytes != -1) {
+ // Since field data is being unloaded (due to expiration or manual
+ // clearing), we also need to decrement the used bytes in the breaker
+ breakerService.getBreaker().addWithoutBreaking(-sizeInBytes);

This comment has been minimized.

@imotov

imotov Dec 13, 2013

Member

That just doesn't feel right. I am not totally sure why yet, but I can reproduce it. When you run this script on an empty one-node cluster it returns:

      "fielddata_breaker" : {
        "maximum_size_in_bytes" : -1,
        "maximum_size" : "-1b",
        "estimated_size_in_bytes" : -4,
        "estimated_size" : "-4b",
        "overhead" : 1.03
      }
@imotov

imotov Dec 13, 2013

Member

That just doesn't feel right. I am not totally sure why yet, but I can reproduce it. When you run this script on an empty one-node cluster it returns:

      "fielddata_breaker" : {
        "maximum_size_in_bytes" : -1,
        "maximum_size" : "-1b",
        "estimated_size_in_bytes" : -4,
        "estimated_size" : "-4b",
        "overhead" : 1.03
      }

This comment has been minimized.

@dakrone

dakrone Dec 17, 2013

Member

Hah, found this issue, rounding error where I was truncating instead of using Math.round(), I'll fix this.

@dakrone

dakrone Dec 17, 2013

Member

Hah, found this issue, rounding error where I was truncating instead of using Math.round(), I'll fix this.

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

can we have an assert here to make sure that we never less than 0? maybe in the addWithoutBreaking method?

@s1monw

s1monw Jan 2, 2014

Contributor

can we have an assert here to make sure that we never less than 0? maybe in the addWithoutBreaking method?

This comment has been minimized.

@dakrone

dakrone Jan 2, 2014

Member

Certainly :) I've been running the tests locally with an assert anyway, so might as well commit it.

@dakrone

dakrone Jan 2, 2014

Member

Certainly :) I've been running the tests locally with an assert anyway, so might as well commit it.

@imotov

View changes

...ava/org/elasticsearch/indices/fielddata/cache/IndicesFieldDataCache.java
@@ -97,6 +100,12 @@ public IndexFieldDataCache buildIndexFieldDataCache(@Nullable IndexService index
return new IndexFieldCache(indexService, index, fieldNames, fieldDataType);
}
+ public IndexFieldDataCache buildIndexFieldDataCache(@Nullable IndexService indexService, Index index, FieldMapper.Names fieldNames,

This comment has been minimized.

@imotov

imotov Dec 13, 2013

Member

It doesn't seem to be used.

@imotov

imotov Dec 13, 2013

Member

It doesn't seem to be used.

This comment has been minimized.

@dakrone

dakrone Dec 17, 2013

Member

I'll remove this.

@dakrone

dakrone Dec 17, 2013

Member

I'll remove this.

@imotov

View changes

...ava/org/elasticsearch/cluster/settings/ClusterDynamicSettingsModule.java
@@ -75,6 +76,8 @@ public ClusterDynamicSettingsModule() {
clusterDynamicSettings.addDynamicSetting(DiskThresholdDecider.CLUSTER_ROUTING_ALLOCATION_DISK_THRESHOLD_ENABLED);
clusterDynamicSettings.addDynamicSetting(InternalClusterInfoService.INTERNAL_CLUSTER_INFO_UPDATE_INTERVAL, Validator.TIME);
clusterDynamicSettings.addDynamicSetting(SnapshotInProgressAllocationDecider.CLUSTER_ROUTING_ALLOCATION_SNAPSHOT_RELOCATION_ENABLED);
+ clusterDynamicSettings.addDynamicSetting(InternalCircuitBreakerService.CIRCUIT_BREAKER_MAX_BYTES_SETTING, Validator.BYTES_SIZE);
+ clusterDynamicSettings.addDynamicSetting(InternalCircuitBreakerService.CIRCUIT_BREAKER_OVERHEAD_SETTING, Validator.DOUBLE);

This comment has been minimized.

@imotov

imotov Dec 13, 2013

Member

Should values lower than 1.0 be allowed here?

@imotov

imotov Dec 13, 2013

Member

Should values lower than 1.0 be allowed here?

This comment has been minimized.

@dakrone

dakrone Dec 13, 2013

Member

Absolutely! For instance, if the circuit breaker is continually estimating too high, you may want to set the overhead to 0.75 in order to decrease the final estimate number.

@dakrone

dakrone Dec 13, 2013

Member

Absolutely! For instance, if the circuit breaker is continually estimating too high, you may want to set the overhead to 0.75 in order to decrease the final estimate number.

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Dec 19, 2013

Member

Pushed a new version of the circuit breaker that addresses @imotov's comments.

Member

dakrone commented Dec 19, 2013

Pushed a new version of the circuit breaker that addresses @imotov's comments.

@kimchy

View changes

...asticsearch/indices/fielddata/breaker/InternalCircuitBreakerService.java
+public class InternalCircuitBreakerService extends AbstractLifecycleComponent<InternalCircuitBreakerService> implements CircuitBreakerService {
+
+ public static final String CIRCUIT_BREAKER_MAX_BYTES_SETTING = "indices.fielddata.cache.breaker.limit";
+ public static final String CIRCUIT_BREAKER_OVERHEAD_SETTING = "indices.fielddata.cache.breaker.overhead";

This comment has been minimized.

@kimchy

kimchy Dec 24, 2013

Member

the settings names do not match the package, it should be indices.fielddata.breaker.xxx

@kimchy

kimchy Dec 24, 2013

Member

the settings names do not match the package, it should be indices.fielddata.breaker.xxx

This comment has been minimized.

@dakrone

dakrone Dec 24, 2013

Member

okay, I'll change those.

@dakrone

dakrone Dec 24, 2013

Member

okay, I'll change those.

@kimchy

View changes

src/main/java/org/elasticsearch/search/SearchModule.java
@@ -68,5 +70,7 @@ protected void configure() {
bind(HighlightPhase.class).asEagerSingleton();
bind(SearchServiceTransportAction.class).asEagerSingleton();
+
+ bind(CircuitBreakerService.class).to(InternalCircuitBreakerService.class).asEagerSingleton();

This comment has been minimized.

@kimchy

kimchy Dec 24, 2013

Member

I would register the circuit breaker in its own module, and make sure to register in on the node level?

@kimchy

kimchy Dec 24, 2013

Member

I would register the circuit breaker in its own module, and make sure to register in on the node level?

This comment has been minimized.

@kimchy

kimchy Dec 24, 2013

Member

actually, probably register it in the field data module?

@kimchy

kimchy Dec 24, 2013

Member

actually, probably register it in the field data module?

This comment has been minimized.

@dakrone

dakrone Dec 24, 2013

Member

@kimchy I tried registering this in the IndexFieldDataModule, but then I run into Guice errors trying to run the tests because no implementation can be found. Is there something I need to do additionally to move it? I'm not very familiar with Guice.

@dakrone

dakrone Dec 24, 2013

Member

@kimchy I tried registering this in the IndexFieldDataModule, but then I run into Guice errors trying to run the tests because no implementation can be found. Is there something I need to do additionally to move it? I'm not very familiar with Guice.

+public class MemoryCircuitBreaker {
+
+ private final long memoryBytesLimit;
+ private final double overheadConstant;

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

what is the reason to have a double here instead of a long? I don't think we should us a double here?

@s1monw

s1monw Dec 27, 2013

Contributor

what is the reason to have a double here instead of a long? I don't think we should us a double here?

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

Because the default is 1.03, and it can be changed to 0.5, 1.8281231, 3.14159, etc based on whether the CB is estimating too high or two low, changing it to a long would remove all granularity.

@dakrone

dakrone Dec 27, 2013

Member

Because the default is 1.03, and it can be changed to 0.5, 1.8281231, 3.14159, etc based on whether the CB is estimating too high or two low, changing it to a long would remove all granularity.

@s1monw

View changes

src/main/java/org/elasticsearch/index/fielddata/ShardFieldData.java
@@ -89,6 +93,15 @@ public void onUnload(FieldMapper.Names fieldNames, FieldDataType fieldDataType,
evictionsMetric.inc();
}
if (sizeInBytes != -1) {
+ // Since field data is being unloaded (due to expiration or manual
+ // clearing), we also need to decrement the used bytes in the breaker
+ synchronized (this) {

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

as far as I saw the breaker has a thread safe implementation that uses AtomicDouble and CMS why is this synchronized?

@s1monw

s1monw Dec 27, 2013

Contributor

as far as I saw the breaker has a thread safe implementation that uses AtomicDouble and CMS why is this synchronized?

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

The breaker has a thread safe implementation, but in this case I'm doing a retrieve, then compare and set outside of the thread safety, so I need to manually synchronize.

@dakrone

dakrone Dec 27, 2013

Member

The breaker has a thread safe implementation, but in this case I'm doing a retrieve, then compare and set outside of the thread safety, so I need to manually synchronize.

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

well this doesn't prevent any other thread from changing the actual value. Since you only sync this part here the addWithoutBreaking method can still be called from other threads concurrently. I think you need a special method that does that. I just wonder how we can actually be below 0 this is almost certainly a bug no?

@s1monw

s1monw Dec 27, 2013

Contributor

well this doesn't prevent any other thread from changing the actual value. Since you only sync this part here the addWithoutBreaking method can still be called from other threads concurrently. I think you need a special method that does that. I just wonder how we can actually be below 0 this is almost certainly a bug no?

@s1monw

View changes

...a/org/elasticsearch/index/fielddata/plain/PackedArrayIndexFieldData.java
+ if (termsEnum == null) {
+ // If the termsEnum is null we were unable to wrap it or get
+ // an iterator, so no need to unwind the breaker
+ } else if (data == null) {

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

I usually to this with a dedicated boolean like this:

boolean success = false;
try {

  success = true;
  return foo;
} finally {
  if (!success) {
    // handle exception
  } else {
    // handle common case
  }
}

maybe this would be more self documenting

@s1monw

s1monw Dec 27, 2013

Contributor

I usually to this with a dedicated boolean like this:

boolean success = false;
try {

  success = true;
  return foo;
} finally {
  if (!success) {
    // handle exception
  } else {
    // handle common case
  }
}

maybe this would be more self documenting

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

I still need to check for nulls though since this is a 3-way check, or are you suggesting two booleans for each "success" status?

@dakrone

dakrone Dec 27, 2013

Member

I still need to check for nulls though since this is a 3-way check, or are you suggesting two booleans for each "success" status?

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

well, you can assign the termsEnum outside of the try / finally and make it final so that case is just gone then. Now we are left with success or failure so that should work again?

@s1monw

s1monw Dec 27, 2013

Contributor

well, you can assign the termsEnum outside of the try / finally and make it final so that case is just gone then. Now we are left with success or failure so that should work again?

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

Gotcha, I'll do that.

@dakrone

dakrone Dec 27, 2013

Member

Gotcha, I'll do that.

@s1monw

View changes

...va/org/elasticsearch/index/fielddata/plain/PagedBytesIndexFieldData.java
try {
// 0 is reserved for "unset"
bytes.copyUsingLengthPrefix(new BytesRef());
- TermsEnum termsEnum = filter(terms, reader);

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

where do you apply the filter now since you pass the terms directly into the beforeLoad method? I must have missed it..

@s1monw

s1monw Dec 27, 2013

Contributor

where do you apply the filter now since you pass the terms directly into the beforeLoad method? I must have missed it..

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

nevermind I saw it in the estimator :) 👍

@s1monw

s1monw Dec 27, 2013

Contributor

nevermind I saw it in the estimator :) 👍

@s1monw

View changes

...va/org/elasticsearch/index/fielddata/plain/PagedBytesIndexFieldData.java
+ long totalBytes = totalTermBytes + (2 * terms.size()) + (4 * terms.getSumDocFreq());
+ return totalBytes;
+ }
+ } catch (Throwable e) {

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

not sure if we shoudl catch Throwable here - maybe it's better to catch a more specialized exception?

@s1monw

s1monw Dec 27, 2013

Contributor

not sure if we shoudl catch Throwable here - maybe it's better to catch a more specialized exception?

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

I'm not sure why it would be helpful? I don't want any exceptions to leak out of the estimations, I just want it to log and message if anything exceptional happens.

@dakrone

dakrone Dec 27, 2013

Member

I'm not sure why it would be helpful? I don't want any exceptions to leak out of the estimations, I just want it to log and message if anything exceptional happens.

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

I just want to prevent that we hide/swallow an OutOfMemoryError here and just continue. Maybe we should catch RuntimeException as well as Exception

@s1monw

s1monw Dec 27, 2013

Contributor

I just want to prevent that we hide/swallow an OutOfMemoryError here and just continue. Maybe we should catch RuntimeException as well as Exception

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

Okay, how about just catching Exception then, that should catch both Exceptions and RuntimeExceptions and let OOMEs through?

@dakrone

dakrone Dec 27, 2013

Member

Okay, how about just catching Exception then, that should catch both Exceptions and RuntimeExceptions and let OOMEs through?

+ final Terms fieldTerms = fields.terms(getFieldNames().indexName());
+
+ if (fieldTerms instanceof BlockTreeTermsReader.FieldReader) {
+ final BlockTreeTermsReader.Stats stats = ((BlockTreeTermsReader.FieldReader) fieldTerms).computeStats();

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

do we have any numbers on how much time this takes?

@s1monw

s1monw Dec 27, 2013

Contributor

do we have any numbers on how much time this takes?

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

I wonder because this goes through all terms and calcs stats so it has a cost...

@s1monw

s1monw Dec 27, 2013

Contributor

I wonder because this goes through all terms and calcs stats so it has a cost...

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

Good point, in my tests it has "seemed" fast, but I will get some concrete numbers so we can have an idea of the overhead.

@dakrone

dakrone Dec 27, 2013

Member

Good point, in my tests it has "seemed" fast, but I will get some concrete numbers so we can have an idea of the overhead.

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

cool! thanks

@s1monw

s1monw Dec 27, 2013

Contributor

cool! thanks

This comment has been minimized.

@@ -0,0 +1,111 @@
+/*
+ * Licensed to ElasticSearch and Shay Banon under one

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

I like the tests etc. but I think we should have some tests where we actually test if we are consistent if we fail to load the fielddata etc. lets hangout on this next week to make sure we have all the tests in place for this.!

@s1monw

s1monw Dec 27, 2013

Contributor

I like the tests etc. but I think we should have some tests where we actually test if we are consistent if we fail to load the fielddata etc. lets hangout on this next week to make sure we have all the tests in place for this.!

This comment has been minimized.

@dakrone

dakrone Dec 27, 2013

Member

+1 sounds good, I think you said you had a way to force exceptions during TermsEnum loading?

@dakrone

dakrone Dec 27, 2013

Member

+1 sounds good, I think you said you had a way to force exceptions during TermsEnum loading?

This comment has been minimized.

@s1monw

s1monw Dec 27, 2013

Contributor

yeah that needs some work though! I hope I can push something tomorrow night (my night)

@s1monw

s1monw Dec 27, 2013

Contributor

yeah that needs some work though! I hope I can push something tomorrow night (my night)

This comment has been minimized.

@s1monw

s1monw Dec 28, 2013

Contributor

I added all the infrastructure needed in this commit: 3113203

take a look at it and if you have questions lemme know!

@s1monw

s1monw Dec 28, 2013

Contributor

I added all the infrastructure needed in this commit: 3113203

take a look at it and if you have questions lemme know!

@s1monw

View changes

src/main/java/org/elasticsearch/index/fielddata/ShardFieldData.java
+ // Since field data is being unloaded (due to expiration or manual
+ // clearing), we also need to decrement the used bytes in the breaker
+ // Only if it's one of the classes that we currently estimate for.
+ if (fieldData instanceof PackedArrayAtomicFieldData || fieldData instanceof PagedBytesAtomicFieldData) {

This comment has been minimized.

@s1monw

s1monw Dec 31, 2013

Contributor

I think we should have a method on the field data itself that gets called on unload passing the breaker? this is safer!

@s1monw

s1monw Dec 31, 2013

Contributor

I think we should have a method on the field data itself that gets called on unload passing the breaker? this is safer!

+ do {
+ currentUsed = this.used.get();
+ newUsed = currentUsed + bytes;
+ long newUsedWithOverhead = (long)(newUsed * overheadConstant);

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

this is much better! I like!

@s1monw

s1monw Jan 2, 2014

Contributor

this is much better! I like!

@s1monw

View changes

.../elasticsearch/indices/fielddata/breaker/CircuitBreakerServiceTests.java
+ try {
+
+ // index 1000 different terms so we have some field data for loading
+ for (long id = 0; id < 1000; id++) {

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

can we do something like int docs = atLeast(300) instead of fixed 1000? just a bit more randomness

@s1monw

s1monw Jan 2, 2014

Contributor

can we do something like int docs = atLeast(300) instead of fixed 1000? just a bit more randomness

+ .setSource(MapBuilder.<String, Object>newMapBuilder().put("test", "value" + id).map()).execute().actionGet();
+ }
+
+ // refresh

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

there is a refresh() shortcut in ElasticsearchIntegationTest

@s1monw

s1monw Jan 2, 2014

Contributor

there is a refresh() shortcut in ElasticsearchIntegationTest

Add field data memory circuit breaker.
This adds the field data circuit breaker, which is used to estimate
the amount of memory required to load field data before loading it. It
then raises a CircuitBreakingException if the limit is exceeded.

It is configured with two parameters:

`indices.fielddata.cache.breaker.limit` - the maximum number of bytes
of field data to be loaded before circuit breaking. Defaults to
`indices.fielddata.cache.size` if set, unbounded otherwise.

`indices.fielddata.cache.breaker.overhead` - a contast for all field
data estimations to be multiplied with before aggregation. Defaults to
1.03.

Both settings can be configured dynamically using the cluster update
settings API.
@s1monw

View changes

...search/indices/fielddata/breaker/RandomExceptionCircuitBreakerTests.java
+ startObject("type").
+ startObject("properties").
+ startObject("test")
+ .field("type", "string")

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

can we have some more types like long / double etc. as well as use a random field_data impl? We could have something like

 .field("type", "string")
 .startObject("fielddata")
 .field("format", randomStringFieldDataFormat())

and something like this for numeric as well:

private static String randomNumericFieldDataFormat() {
        return randomFrom(Arrays.asList("array", "compressed", "doc_values"));
}
private static String randomBytesFieldDataFormat() {
        return randomFrom(Arrays.asList("paged_bytes", "fst", "doc_values"));
}

I guess we can add those to ElasticsearchIntegrationTest

@s1monw

s1monw Jan 2, 2014

Contributor

can we have some more types like long / double etc. as well as use a random field_data impl? We could have something like

 .field("type", "string")
 .startObject("fielddata")
 .field("format", randomStringFieldDataFormat())

and something like this for numeric as well:

private static String randomNumericFieldDataFormat() {
        return randomFrom(Arrays.asList("array", "compressed", "doc_values"));
}
private static String randomBytesFieldDataFormat() {
        return randomFrom(Arrays.asList("paged_bytes", "fst", "doc_values"));
}

I guess we can add those to ElasticsearchIntegrationTest

@s1monw

View changes

...search/indices/fielddata/breaker/RandomExceptionCircuitBreakerTests.java
+ for (int i = 0; i < numDocs ; i++) {
+ try {
+ client().prepareIndex("test", "type", "" + i)
+ .setTimeout(TimeValue.timeValueSeconds(1)).setSource("test", English.intToEnglish(i)).get();

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

I guess it would be better to have a random unicode string here instead of the English? We don't need hits here really

@s1monw

s1monw Jan 2, 2014

Contributor

I guess it would be better to have a random unicode string here instead of the English? We don't need hits here really

@s1monw

View changes

...search/indices/fielddata/breaker/RandomExceptionCircuitBreakerTests.java
+ // successfully set back to zero. If there is a bug in the circuit
+ // breaker adjustment code, it should show up here by the breaker
+ // estimate being either positive or negative.
+ client().admin().indices().prepareClearCache("test").setFieldDataCache(true).execute().actionGet();

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

can we randomly skip this test and keep the loaded or not loaded one?

@s1monw

s1monw Jan 2, 2014

Contributor

can we randomly skip this test and keep the loaded or not loaded one?

+
+ public static final String EXCEPTION_TOP_LEVEL_RATIO_KEY = "index.engine.exception.ratio.top";
+ public static final String EXCEPTION_LOW_LEVEL_RATIO_KEY = "index.engine.exception.ratio.low";
+

This comment has been minimized.

@s1monw

s1monw Jan 2, 2014

Contributor

please put a TODO here that I generalize this class and add it as a util! :)

@s1monw

s1monw Jan 2, 2014

Contributor

please put a TODO here that I generalize this class and add it as a util! :)

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Jan 2, 2014

Contributor

LGTM please squash and push 👍

Contributor

s1monw commented Jan 2, 2014

LGTM please squash and push 👍

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Jan 2, 2014

Member

Merged in a754224, closing #4592

Member

dakrone commented Jan 2, 2014

Merged in a754224, closing #4592

@dakrone dakrone closed this Jan 2, 2014

@dakrone dakrone deleted the dakrone:circuit-breaker-squashed branch Apr 21, 2014

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