Fix DruidNavigator loading multiple tables #309

kevinhinterlong · 2017-05-26T18:09:23Z

It appears the locking mechanism was forced to time out while loading
multiple tables because the lock for getting all the tables was still active.

Now only the lock for getting all the tables (loadAllDatasources()) is used.

cdeszaq

A few things here:

Changelog entry needed (probably in the Fixes section)
It would be nice to have a test in place that shows the issue and that this change fixes it.
I'm not sure I understand how this is fixing the problem. The main change I see is to move the get to the caller. I don't really understand how that solves the problem which, as I understand it, is that the follow-on calls are trying to run while the 1st call isn't finished. Given how the AsyncHttpClient works, multiple calls in flight at once shouldn't be a problem. And there's no lock involved (other than the implicit one in Future::get), so I'm a little confused there as well.
Don't we still want to have the later calls timeout or at least finish executing before the rest of the application starts up? I think this change would allow the app to start w/o having any of the tables configured...

michael-mclawhorn · 2017-06-01T18:32:07Z

...e/src/main/java/com/yahoo/wiki/webservice/data/config/dimension/GenericDimensionConfigs.java

 import java.util.function.Supplier;
 import java.util.stream.Collectors;

 /**
 * Hold all the dimension configurations for a generic druid configuration.
 */
 public class GenericDimensionConfigs {
-    private final Set<DimensionConfig> dimensionConfigs;
+    private final Map<String, Set<DimensionConfig>> dimensionConfigs;



It's not clear what the String in this map is.

QubitPi · 2017-06-01T19:18:38Z

...roovy/com/yahoo/wiki/webservice/web/endpoints/MultipleDatasourceDruidConfigLoaderSpec.groovy

+            } else if (druidWebService.lastUrl.equals("/datasources/table2/?full")) {
+                return getFullTable(datasources[1], table2_metrics, table2_dimensions)
+            }
+            println "No response for " + druidWebService.lastUrl


Maybe you forgot to delete it?

QubitPi · 2017-06-01T19:19:09Z

...rc/test/groovy/com/yahoo/wiki/webservice/web/endpoints/AutomaticDruidConfigLoaderSpec.groovy

@@ -53,7 +52,7 @@ public class AutomaticDruidConfigLoaderSpec extends Specification {
            } else if (druidWebService.lastUrl == '/datasources/' + datasource + "/?full") {
                return expectedMetricsAndDimensions
            }
-            return "Unexpected URL"
+            println "No response for " + druidWebService.lastUrl


Maybe you forgot to delete this?

QubitPi · 2017-06-01T19:20:14Z

...e/src/main/java/com/yahoo/wiki/webservice/data/config/dimension/GenericDimensionConfigs.java

+
+                    dimensionConfigs.put(dataSourceConfiguration.getName(), tableDimensionConfigs);
+                });
+        ;


What's this for?

QubitPi · 2017-06-01T19:21:26Z

...e/src/main/java/com/yahoo/wiki/webservice/data/config/dimension/GenericDimensionConfigs.java

+    /**
+     * Get all the dimension configurations associated with this datasource.
+     *
+     * @param dataSourceConfiguration the datasource configuration's dimensions to load


Two spaces before description. Capitalize the

QubitPi · 2017-06-01T19:21:38Z

...ic-example/src/main/java/com/yahoo/wiki/webservice/data/config/table/GenericTableLoader.java

-import java.util.List;
-import java.util.Set;
+import javax.validation.constraints.NotNull;
+import java.util.*;


Avoid start import

QubitPi · 2017-06-01T19:22:53Z

...roovy/com/yahoo/wiki/webservice/web/endpoints/MultipleDatasourceDruidConfigLoaderSpec.groovy

+    ]
+}
+
+"""


Align indentation

michael-mclawhorn

Looks pretty good

cdeszaq

Partial review. Didn't get as far as GenericDimensionConfigs.

cdeszaq · 2017-06-06T18:49:59Z

fili-generic-example/README.md

@@ -90,4 +95,6 @@ Here are some sample queries that you can run to verify your server:

 1. In IntelliJ, go to `File -> Open`
 2. Select the `pom.xml` file at the root of the project
-3. Run `GenericMain` which can be found in `fili-generic-example` (e.g. right click and choose run)
+3. Under `src/main/resources/applicationConfig.properties`, change `bard__non_ui_druid_broker`, 
+`bard__ui_druid_broker`, `bard__druid_coord`, and other properties as needed.


Are these properties set correctly already for what the Druid wikipedia walkthrough results in? If not, we should set them so that they are probably correct.

Yep these are set correctly for running it locally and they work with the wikipedia walkthrough.

I added the following note in README.md to clarify:

NOTE: if you're running this locally and haven't changed any settings (like the Wikipedia example) you can skip step 3.

cdeszaq · 2017-06-06T18:55:16Z

...generic-example/src/main/java/com/yahoo/wiki/webservice/data/config/auto/DruidNavigator.java

 import com.yahoo.bard.webservice.data.time.DefaultTimeGrain;
 import com.yahoo.bard.webservice.data.time.TimeGrain;
 import com.yahoo.bard.webservice.druid.client.DruidWebService;
 import com.yahoo.bard.webservice.druid.client.SuccessCallback;
 import com.yahoo.bard.webservice.util.IntervalUtils;
-
-import com.fasterxml.jackson.databind.JsonNode;
-


This import should stay down here.

cdeszaq · 2017-06-06T20:16:13Z

...generic-example/src/main/java/com/yahoo/wiki/webservice/data/config/auto/DruidNavigator.java

@@ -69,15 +67,35 @@ public DruidNavigator(DruidWebService druidWebService) {
     * ["wikiticker"]
     */
    private void loadAllDatasources() {
-        queryDruid(rootNode -> {
+        final List<Future<Response>> fullTableResponses = new ArrayList<>();


Doesn't need to be final

kevinhinterlong · 2017-06-09T18:36:05Z

I added a note in the readme of the version this was last tested with.
I think this should be good to merge now.

garyluoex

Rebase, and Im ok with it

It appears the locking mechanism was forced to time out while loading multiple tables

- This also adds a test to make sure that multiple tables get configured properly. - There was also an issue with the GenericTableLoader showing the same metrics, granularities, etc for all tables

Now dimensions show up only in the correct table. Also made a few fields final.

- The druid dimension loader is now causing it to fail because the tables can not be found in the DataSourceMetadataService - Enabled DruidDimensionLoader

- Also fix list of required settings to top of readme

No here

cdeszaq previously requested changes May 26, 2017

View reviewed changes

QubitPi added the NEED CHANGES label May 26, 2017

QubitPi self-assigned this May 26, 2017

michael-mclawhorn reviewed Jun 1, 2017

View reviewed changes

QubitPi suggested changes Jun 1, 2017

View reviewed changes

michael-mclawhorn approved these changes Jun 2, 2017

View reviewed changes

QubitPi removed the NEED CHANGES label Jun 6, 2017

QubitPi approved these changes Jun 6, 2017

View reviewed changes

cdeszaq reviewed Jun 6, 2017

View reviewed changes

kevinhinterlong force-pushed the fixMultipleTableConfiguration branch from 0b71c98 to 558fe3f Compare June 8, 2017 20:41

yahoo deleted a comment Jun 8, 2017

yahoo deleted a comment Jun 9, 2017

kevinhinterlong added the MERGEABLE label Jun 9, 2017

garyluoex approved these changes Jun 22, 2017

View reviewed changes

kevinhinterlong added 11 commits June 22, 2017 13:14

DruidNavigator locked up while loading several tables

62e7525

It appears the locking mechanism was forced to time out while loading multiple tables

Fix javadoc, revert timeout to 30 seconds

8f62462

Fix DruidNavigator loading multiple tables

3646fc1

- This also adds a test to make sure that multiple tables get configured properly. - There was also an issue with the GenericTableLoader showing the same metrics, granularities, etc for all tables

Fix issue where multiple tables load the same dimension name

a996d2f

Fix check style errors

bb282ab

Fix bug where all dimensions displayed for all tables.

455c25e

Now dimensions show up only in the correct table. Also made a few fields final.

Fix refactoring which didn't get noticed in a test spec

246f21a

Clarify variable names

a5b784a

Address Jiaqi's comments, add changelog, fix readme

5a24c31

- The druid dimension loader is now causing it to fail because the tables can not be found in the DataSourceMetadataService - Enabled DruidDimensionLoader

Clarify instructions in the readme

76b130c

Address Rick's comments and clarify readme for Wikipedia Example

0780803

Add a note in README for last working version

b6d2420

- Also fix list of required settings to top of readme

kevinhinterlong force-pushed the fixMultipleTableConfiguration branch from 4b13f9a to b6d2420 Compare June 22, 2017 18:14

yahoo deleted a comment Jun 22, 2017

garyluoex merged commit 0e599c9 into yahoo:master Jun 22, 2017

kevinhinterlong deleted the fixMultipleTableConfiguration branch August 8, 2017 14:52

QubitPi mentioned this pull request Jan 7, 2018

Enhance generic example #610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DruidNavigator loading multiple tables #309

Fix DruidNavigator loading multiple tables #309

kevinhinterlong commented May 26, 2017

cdeszaq left a comment •

edited

Loading

michael-mclawhorn Jun 1, 2017

QubitPi Jun 1, 2017

QubitPi Jun 1, 2017

QubitPi Jun 1, 2017

QubitPi Jun 1, 2017

QubitPi Jun 1, 2017

QubitPi Jun 1, 2017

michael-mclawhorn left a comment

cdeszaq left a comment

cdeszaq Jun 6, 2017

kevinhinterlong Jun 8, 2017

cdeszaq Jun 6, 2017

cdeszaq Jun 6, 2017

kevinhinterlong commented Jun 9, 2017

garyluoex left a comment

+                  ]
+              }
+              """

Fix DruidNavigator loading multiple tables #309

Fix DruidNavigator loading multiple tables #309

Conversation

kevinhinterlong commented May 26, 2017

cdeszaq left a comment • edited Loading

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

cdeszaq 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

kevinhinterlong commented Jun 9, 2017

garyluoex left a comment

Choose a reason for hiding this comment

cdeszaq left a comment •

edited

Loading