Allow use of non-threadsafe ObjectCachingColumnSelectorFactory by xanec · Pull Request #4397 · apache/druid

xanec · 2017-06-13T02:35:20Z

Added am additional boolean parameter in IncrementalIndex constructor (and subsequently the constructors for its subclasses OnheapIncrementalIndex and OffheapIncrementalIndex) and the protected function initAggs to indicate if ConcurrentMap should be used.

…d not be threadsafe.

leventov · 2017-06-13T02:56:06Z

@@ -366,14 +390,24 @@ public void close()
  // here its made thread safe to support the special case of groupBy where the multiple threads can add concurrently to the IncrementalIndex.
  static class ObjectCachingColumnSelectorFactory implements ColumnSelectorFactory
  {


Could you please also change code of makeXxxColumnSelector methods to use computeIfAbsent() instead of putIfAbsent()?

leventov · 2017-06-13T03:00:14Z

@@ -91,6 +92,26 @@ public OffheapIncrementalIndex(
    aggBuffers.add(bb);
  }



There are no compatibility concerns, so suggested to inline all usages of this constructor and remove it.

I have a similar constructor for OnheapIncrementalIndex, should I inline it as well?

Yes. Also having 4 boolean parameters in a row on the call sites is dangerous (to accidentally switch them) and not readable, consider replacing one or two of them with enums of two options.

Yeah, I thought so too. Actually, I was considering creating Builders for On/OffheapIncrementalIndex, would that be better?

I think yes

Okay, I will go with that.

leventov · 2017-06-13T03:03:36Z

+
+      if (concurrentEventAdd) {
+        longColumnSelectorMap = Maps.newConcurrentMap();
+        floatColumnSelectorMap = Maps.newConcurrentMap();


Please use ConcurrentHashMap, the implementation behind Maps.newConcurrentMap() has higher overhead and is less scalable.

jihoonson · 2017-06-13T03:05:59Z

It would be nice if you add docs to somewhere when IncrementalIndex must be thread-safe or not.

xanec · 2017-06-13T15:26:49Z

Hi @jihoonson, it's already stated together with ObjectCachingColumnSelectorFactory:

// Caches references to selector objects for each column instead of creating a new object each time in order to save heap space.
// In general the selectorFactory need not to thread-safe.
// here its made thread safe to support the special case of groupBy where the multiple threads can add concurrently to the IncrementalIndex.

But I will add in additional remarks/docs together with the declaration of the parameter.

- Replace Maps.newXXXMap() with normal instantiation - Documentations on when is thread-safe required. - Use Builders for On/OffheapIncrementalIndex

leventov · 2017-06-14T21:06:51Z

+                .withRollup(IncrementalIndexSchema.DEFAULT_ROLLUP)
+                .build()
+        )
+        .setMaxRowCount(1000)


This test is about groupBy, shouldn't it have .setConcurrentEventAdd(true)?

Sorry, missed that.

leventov · 2017-06-14T21:14:18Z

-      boolean sortFacts,
-      int maxRowCount
-  )
+  public static class Builder


This builder and OffHeapIncrementalIndex.Builder could be the same class with two building methods: buildOffHeap() and buildOnHeap().

Sure, that can be done.

Actually, I have also considered having OffheapIncrementalIndex.Builder to extend the OnheapIncrementalIndex.Builder. Would this be better?

I think overriding doesn't fit here, just IncrementalIndexBuilder with two different building methods is better IMO.

Got it 👍

leventov · 2017-06-14T21:17:19Z

-        );
-        return prev != null ? prev : newSelector;
-      }
+      return floatColumnSelectorMap.computeIfAbsent(columnName, delegate::makeFloatColumnSelector);


computeIfAbsent() when majority of calls if expected to find the entry is present already could be slower. So what I meant is code like this:

FloatColumnSelector existing = floatColumnSelectorMap.get(columnName); if (existing != null) { return existing; } else { floatColumnSelectorMap.computeIfAbsent(columnName, delegate::makeFloatColumnSelector); }

But won't this be reflected in the implementation of computeIfAbsent? The source code of the Map interface as follows:

default V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) { Objects.requireNonNull(mappingFunction); V v; if ((v = get(key)) == null) { V newValue; if ((newValue = mappingFunction.apply(key)) != null) { put(key, newValue); return newValue; } } return v; }

Because of how code is inlined it might be not identical. And for ConcurrentHashMap the difference is bigger, because computeIfAbsent() always locks, and get() just makes a volatile read.

I see, thanks for the explanation.

leventov · 2017-06-14T21:17:36Z

-        );
-        return prev != null ? prev : newSelector;
-      }
+      return longColumnSelectorMap.computeIfAbsent(columnName, delegate::makeLongColumnSelector);


leventov · 2017-06-14T21:17:43Z

-        );
-        return prev != null ? prev : newSelector;
-      }
+      return objectColumnSelectorMap.computeIfAbsent(columnName, delegate::makeObjectColumnSelector);


leventov · 2017-06-14T21:26:58Z

+                    )
+                ).build()
+        )
+        .setMaxRowCount(NUM_POINTS)


setReportParseExceptions(false) should be here

leventov · 2017-06-14T22:19:24Z

+                .withMinTimestamp(0)
+                .withQueryGranularity(Granularities.NONE)
+                .withMetrics(aggs)
+                .withRollup(IncrementalIndexSchema.DEFAULT_ROLLUP)


DEFAULT_ROLLUP (true) is set by default in the builder, don't need to be specified explicitly

leventov · 2017-06-14T22:20:04Z

+    return new OnheapIncrementalIndex.Builder()
+        .setIncrementalIndexSchema(
+            new IncrementalIndexSchema.Builder()
+                .withMinTimestamp(0)


This is the default value, don't need to be specified explicitly.

leventov · 2017-06-14T22:22:12Z

+    return new OnheapIncrementalIndex.Builder()
+        .setIncrementalIndexSchema(
+            new IncrementalIndexSchema.Builder()
+                .withQueryGranularity(Granularities.NONE)


This is the default value, don't need to be specified explicitly.

leventov · 2017-06-14T22:22:20Z

+            new IncrementalIndexSchema.Builder()
+                .withQueryGranularity(Granularities.NONE)
+                .withMetrics(schemaInfo.getAggsArray())
+                .withDimensionsSpec(DimensionsSpec.ofEmpty())


This is the default value, don't need to be specified explicitly.

BTW DimensionsSpec.ofEmpty() is not constant, maybe makes sense to make it so.

- Constant EMPTY DimensionsSpec - Improvement on IncrementalIndexSchema.Builder - Remove setting of default values - Use var args for metrics - Correction on On/OffheapIncrementalIndex Builders - Combine On/OffheapIncrementalIndex Builders

leventov

Please also fix Checkstyle

leventov · 2017-06-15T05:35:44Z

-    return new OnheapIncrementalIndex.Builder()
+    return new IncrementalIndex.Builder()
        .setIncrementalIndexSchema(
            new IncrementalIndexSchema.Builder()


This is a very frequent pattern, suggested to add shortcut like IncrementalIndex.Builder. setSimpleIncrementalIndexSchema(AggregatorFactory...)

Sure but it seems that such usage (i.e., setting only metrics) only occurs in non-productions use (i.e., tests and benchmarks). Should we add in another API only for these uses?

I think it's important to remove boilerplate and repetition from tests as well as from production code, because the need to write a lot of boilerplate demotivates people to write tests. Also you can rename method setIncrementalIndexSchema() to just setIndexSchema() because in the context of IncrementalIndex.Builder it's not needed to repeat that the index schema is "incremental".

You could call this method setSimpleTestingIndexSchema and annotate @VisibleForTesting to emphasize that it should be used only for testing.

leventov · 2017-06-15T21:30:39Z

      return this;
    }

+    // A helper method to set a simple index schema with only metrics and default values for the other parameters. Note


Duplicate comment

Opps, really sorry about that.

leventov · 2017-06-15T21:31:26Z

+    // that this method is normally used for testing and benchmarking; it is unlikely that you would use it in
+    // production settings.
+
+    /** A helper method to set a simple index schema with only metrics and default values for the other parameters. Note


According to Java style text should start on the next line after /**

Adding a flag to indicate when ObjectCachingColumnSelectorFactory nee…

1085000

…d not be threadsafe.

xanec force-pushed the non-threadsafe-objectcachingcolumnselectorfactory branch from 68413b2 to 1085000 Compare June 13, 2017 02:40

leventov reviewed Jun 13, 2017

View reviewed changes

leventov added the Improvement label Jun 13, 2017

leventov reviewed Jun 13, 2017

View reviewed changes

leventov changed the title ~~[BACKEND-902] Allow use of non-threadsafe ObjectCachingColumnSelectorFactory~~ Allow use of non-threadsafe ObjectCachingColumnSelectorFactory Jun 13, 2017

leventov reviewed Jun 13, 2017

View reviewed changes

- Use of computeIfAbsent over putIfAbsent

2010317

- Replace Maps.newXXXMap() with normal instantiation - Documentations on when is thread-safe required. - Use Builders for On/OffheapIncrementalIndex

leventov requested changes Jun 14, 2017

View reviewed changes

- Optimization on computeIfAbsent

71516ab

- Constant EMPTY DimensionsSpec - Improvement on IncrementalIndexSchema.Builder - Remove setting of default values - Use var args for metrics - Correction on On/OffheapIncrementalIndex Builders - Combine On/OffheapIncrementalIndex Builders

leventov reviewed Jun 15, 2017

View reviewed changes

xanec added 2 commits June 15, 2017 07:47

- Removing unused imports.

ee36d88

- Helper method for testing with IncrementalIndex.Builder

6e59679

leventov requested changes Jun 15, 2017

View reviewed changes

xanec and others added 2 commits June 15, 2017 14:44

- Correction on javadoc.

4ebd36c

Style fix

7778492

leventov approved these changes Jun 15, 2017

View reviewed changes

leventov merged commit f68a069 into apache:master Jun 16, 2017

leventov deleted the non-threadsafe-objectcachingcolumnselectorfactory branch June 16, 2017 21:04

leventov mentioned this pull request Jan 22, 2019

Prohibit assigning concurrent maps into Map-typed variables and fields and fix a race condition in CoordinatorRuleManager #6898

Merged

		@@ -91,6 +92,26 @@ public OffheapIncrementalIndex(
		aggBuffers.add(bb);
		}

Conversation

xanec commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Jun 13, 2017

Uh oh!

xanec commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

leventov Jun 15, 2017 •

edited

Loading