Generify DruidDimensionLoader #449

kevinhinterlong · 2017-07-27T17:52:35Z

The DruidDimensionLoader now only tries to load dimensions on datasources that contain that dimension.

kevinhinterlong · 2017-07-27T18:02:44Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/DruidDimensionsLoader.java

@@ -130,11 +127,9 @@ public DruidDimensionsLoader(
        // time if `dimensions` were a flat list instead of a list of singleton lists.


Didn't update this comment

kevinhinterlong · 2017-07-27T18:06:22Z

Not sure why I couldn't review this part, but it appears I missed this as well TimeUnit.MILLISECONDS.toMillis(60000)

michael-mclawhorn

If this code were more load bearing I'd ask for more tests on some of the inner details (making sure that columns are loaded on all table that they're on)

But it's not and I don't care that much about that optimization.

kevinhinterlong · 2017-08-02T15:15:28Z

@michael-mclawhorn This is now more of a dramatic change involding the DimensionRowProvider strategy you mentioned so you'll need to re-review.

Basically what changed is

DruidDimensionLoader -> DimensionLoader
DimensionLoader now takes a collection of AbstractDimensionRowProvider in the constructor
The DruidDimensionLoader druid querying logic has been moved to DruidDimensionRowProvider

kevinhinterlong · 2017-08-04T14:45:49Z

It also might be interesting to add a NoOpDimensionRowProvider which just updates all the dimensions without loading any DimensionRows

garyluoex

Most comments are opinions, feel free to discuss if you think otherwise. Besides the opinions, it is goo.

garyluoex · 2017-08-14T15:24:26Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

     */
-    protected DruidDimensionsLoader buildDruidDimensionsLoader(
+    protected DimensionLoader buildDruidDimensionsLoader(


I don't think we need more than one dimension loader per application this point right? @michael-mclawhorn .Would be nice if we can deprecate this method in a backward compatible way so that we can change the name of this method also.

I could mark both buildDruidDimensionsLoader(...) and setupDruidDimensionsLoader(...) as deprecated and make a method to provide the collection of AbstractDimensionRowProvider?

garyluoex · 2017-08-14T15:55:30Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

     *
     * @param healthCheckRegistry  The health check registry to register Dimension lookup health checks
-     * @param dataDruidDimensionsLoader  The DruidDimensionLoader used for monitoring and health checks
+     * @param dataDimensionLoader  The DruidDimensionLoader used for monitoring and health checks
     */
    protected final void setupDruidDimensionsLoader(


If we want to keep two separate method for dimension loader setup, would be nice to change this name also.

garyluoex · 2017-08-14T15:57:26Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

@@ -879,7 +879,7 @@ protected final ConfigurationLoader getConfigurationLoader() {
     * @return A configurationLoader instance
     */
    protected ConfigurationLoader buildConfigurationLoader(
-            DimensionLoader dimensionLoader,
+            com.yahoo.bard.webservice.data.config.dimension.DimensionLoader dimensionLoader,


We should try to rename things anyways, so try to get rid of the conflict by renaming.

garyluoex · 2017-08-14T15:58:11Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractDimensionRowProvider.java

+/**
+ * The AbstractDimensionRowProvider provides a way to load dimension rows onto dimensions.
+ */
+public abstract class AbstractDimensionRowProvider {


DimensionRow or DimensionValue? depends on you, both are fine but personally I think value feels better, the concept of a dimension table is not obvious in fili.

garyluoex · 2017-08-14T18:27:00Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractDimensionRowProvider.java

+    private final List<Dimension> dimensions;
+    private final List<DataSource> dataSources;
+
+    private HttpErrorCallback errorCallback;


This class assumes dimensions are provided through http request? Not sure if we can be a bit more generic or not... I think I would prefer a generic retrieval method ignorant class for this class. @michael-mclawhorn

Yea it's kind of weird to put it here I agree. I thought it would be easier for the DimensionRowProvider implementation to only use it where it's applicable.

garyluoex · 2017-08-14T19:48:58Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractDimensionRowProvider.java

+     * @param dimension  The dimension to load.
+     * @param dataSource  The datasource to query values for.
+     */
+    protected abstract void query(Dimension dimension, DataSource dataSource);


I would put this to the top of method list so that it is more obvious to find.

garyluoex · 2017-08-14T20:02:12Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractDimensionRowProvider.java

+    private HttpErrorCallback errorCallback;
+    private FailureCallback failureCallback;
+    private BiConsumer<Dimension, DimensionRow> dimensionRowConsumer;
+    private Consumer<Dimension> dimensionLoadedConsumer;


I would prefer not to store these two consumers, but instead pass in to function that uses consumer. It feels weird that a provider stores the consumer.

Alternatively I could just pull the two methods that are actually used from DimensionLoader into AbstractDimensionRowProvider since they're unlikely to change

@Override public void accept(Dimension dimension, DimensionRow dimensionRow) { dimension.addDimensionRow(dimensionRow); } @Override public void accept(Dimension dimension) { // Tell the dimension it's been updated dimension.setLastUpdated(DateTime.now()); }

garyluoex · 2017-08-14T21:36:13Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/DimensionLoader.java

+ */
+@Singleton
+public class DimensionLoader extends Loader<Boolean>
+        implements BiConsumer<Dimension, DimensionRow>, Consumer<Dimension> {


Not sure if implementing consumer is useful here, it is unclear that a loader is a consumer.

michael-mclawhorn

The Provider should probably be a Loader.

Suggested fix:
Change interface Loader to LoadTask
Change DimensionValueLoader to DimensionValueLoadTask
Change DimensionValueProvider to DimensionValueLoader

michael-mclawhorn · 2017-08-15T15:26:55Z

...core/src/main/java/com/yahoo/bard/webservice/application/AbstractDimensionValueProvider.java

+    ) {
+        this.dimensions = dimensionsToLoad.stream()
+                .map(dimensionDictionary::findByApiName)
+                .collect(Collectors.toList());


Would this be better as a Set?

michael-mclawhorn · 2017-08-15T15:29:46Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/DimensionValueLoader.java

+    private final Collection<AbstractDimensionValueProvider> dimensionRowProviders;
+
+    /**
+     * DimensionLoader fetches data from Druid and adds it to the dimension cache.


michael-mclawhorn · 2017-08-15T15:29:53Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/DimensionValueLoader.java

+
+/**
+ * DimensionLoader sends requests to the druid search query interface to get a list of dimension
+ * values to add to the dimension cache.


michael-mclawhorn · 2017-08-15T15:30:08Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/DimensionValueLoader.java

+            SYSTEM_CONFIG.getPackageVariableName("druid_dim_loader_timer_duration");
+    public static final String DRUID_DIM_LOADER_TIMER_DELAY_KEY =
+            SYSTEM_CONFIG.getPackageVariableName("druid_dim_loader_timer_delay");
+


Druid in local constant name?

Changing the configuration parameter will require a deprecation.

michael-mclawhorn · 2017-08-15T15:37:12Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/DruidDimensionValueProvider.java

+
+    private static final long TEN_YEARS_MILLIS = 10 * TimeUnit.DAYS.toMillis(365);
+    private static final Duration DURATION = new Duration(TEN_YEARS_MILLIS);
+    private static final Interval INTERVAL = new Interval(DURATION, DateTime.now());


http://joda-time.sourceforge.net/apidocs/org/joda/time/Years.html#years(int)
new Interval (Years.years(10), DateTime.now())

michael-mclawhorn · 2017-08-15T15:42:17Z

...core/src/main/java/com/yahoo/bard/webservice/application/AbstractDimensionValueProvider.java

+     * @param dimensionDictionary  The dimension dictionary to load dimensions from.
+     * @param dimensionsToLoad  The dimensions to use.
+     */
+    public AbstractDimensionValueProvider(


See if you can create an interface for this

michael-mclawhorn · 2017-08-15T23:37:38Z

Resolve conflicts, Add the interface name change to the CHANGE LOG, rebase and then someone can merge at will.

- This fixed the naming schema by moving DimensionRow -> DimensionValue - Removed the abstraction of consumers for loading dimensions

- Loader -> LoadTask - Lots of other refactoring got dragged in - DimensionValueLoader -> DimensionValueLoadTask - DimensionValueProvider -> DimensionValueLoader (interface) - Fix tests

kevinhinterlong added NEED 2 REVIEWS REVIEWABLE labels Jul 27, 2017

kevinhinterlong commented Jul 27, 2017

View reviewed changes

kevinhinterlong force-pushed the cleanupDruidDimensionLoader branch from dfa8c81 to 085d358 Compare July 28, 2017 16:23

kevinhinterlong mentioned this pull request Jul 28, 2017

Stabilizing Travis and Maven builds #454

Merged

michael-mclawhorn approved these changes Jul 28, 2017

View reviewed changes

yahoo deleted a comment Jul 28, 2017

archolewa added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Jul 31, 2017

kevinhinterlong force-pushed the cleanupDruidDimensionLoader branch from 079ae99 to ae84d62 Compare August 2, 2017 15:09

yahoo deleted a comment Aug 2, 2017

kevinhinterlong changed the title ~~Cleanup Druid Dimension Loader~~ Generify DruidDimensionLoader Aug 2, 2017

kevinhinterlong added the NEED REBASE label Aug 4, 2017

kevinhinterlong added this to the Kevin End Date milestone Aug 14, 2017

garyluoex approved these changes Aug 14, 2017

View reviewed changes

yahoo deleted a comment Aug 15, 2017

kevinhinterlong force-pushed the cleanupDruidDimensionLoader branch from 3f4a01a to f379906 Compare August 15, 2017 14:55

kevinhinterlong removed the NEED REBASE label Aug 15, 2017

yahoo deleted a comment Aug 15, 2017

kevinhinterlong force-pushed the cleanupDruidDimensionLoader branch from f379906 to 380b146 Compare August 15, 2017 15:10

michael-mclawhorn requested changes Aug 15, 2017

View reviewed changes

michael-mclawhorn approved these changes Aug 15, 2017

View reviewed changes

kevinhinterlong added 7 commits August 15, 2017 18:46

Cleanup Druid Dimension Loader

85edf57

Address my own comments ✋

1cc7be6

Generify the DruidDimensionLoader

a7af922

Fix usages of DruidDimensionLoader and rename -> DimensionLoader

7ac5eee

Address comments

5bc1861

- This fixed the naming schema by moving DimensionRow -> DimensionValue - Removed the abstraction of consumers for loading dimensions

Remove unused imports and add changelog

cdcaebb

Address comments

cbcc50c

- Loader -> LoadTask - Lots of other refactoring got dragged in - DimensionValueLoader -> DimensionValueLoadTask - DimensionValueProvider -> DimensionValueLoader (interface) - Fix tests

kevinhinterlong force-pushed the cleanupDruidDimensionLoader branch from 839ee60 to cbcc50c Compare August 15, 2017 23:49

kevinhinterlong added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels Aug 15, 2017

garyluoex merged commit 5de3050 into yahoo:master Aug 16, 2017

kevinhinterlong deleted the cleanupDruidDimensionLoader branch August 16, 2017 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generify DruidDimensionLoader #449

Generify DruidDimensionLoader #449

kevinhinterlong commented Jul 27, 2017

kevinhinterlong Jul 27, 2017

kevinhinterlong commented Jul 27, 2017

michael-mclawhorn left a comment

kevinhinterlong commented Aug 2, 2017 •

edited

Loading

kevinhinterlong commented Aug 4, 2017

garyluoex left a comment

garyluoex Aug 14, 2017

kevinhinterlong Aug 15, 2017

garyluoex Aug 14, 2017

garyluoex Aug 14, 2017

garyluoex Aug 14, 2017

garyluoex Aug 14, 2017

kevinhinterlong Aug 14, 2017

garyluoex Aug 14, 2017

garyluoex Aug 14, 2017

kevinhinterlong Aug 14, 2017

garyluoex Aug 14, 2017

michael-mclawhorn left a comment

michael-mclawhorn Aug 15, 2017

michael-mclawhorn Aug 15, 2017

michael-mclawhorn Aug 15, 2017

michael-mclawhorn Aug 15, 2017

michael-mclawhorn Aug 15, 2017

michael-mclawhorn Aug 15, 2017

michael-mclawhorn commented Aug 15, 2017

		@@ -130,11 +127,9 @@ public DruidDimensionsLoader(
		// time if `dimensions` were a flat list instead of a list of singleton lists.

Generify DruidDimensionLoader #449

Generify DruidDimensionLoader #449

Conversation

kevinhinterlong commented Jul 27, 2017

Choose a reason for hiding this comment

kevinhinterlong commented Jul 27, 2017

michael-mclawhorn left a comment

Choose a reason for hiding this comment

kevinhinterlong commented Aug 2, 2017 • edited Loading

kevinhinterlong commented Aug 4, 2017

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-mclawhorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-mclawhorn commented Aug 15, 2017

kevinhinterlong commented Aug 2, 2017 •

edited

Loading