Prepare For Partial Data V2 #264

QubitPi · 2017-05-02T16:54:38Z

1st PR implementing #215

cdeszaq · 2017-05-03T16:30:00Z

fili-core/src/main/resources/moduleConfig.properties

@@ -124,6 +124,9 @@ bard__default_asyncAfter=never
 # Flag to turn on case sensitive keys in keyvalue store
 bard__case_sensitive_keys_enabled = false

+# Default value for Druid limit on uncovered interval


This is obviously the default because it's in this file. Instead, the comment should describe what the parameter is for, what it controls, and what this default value means or does.

cdeszaq · 2017-05-03T16:30:57Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java

+    TOO_MUCH_INTERVAL_MISSING(
+            "More than %s interval missing information received from druid, inspect if query " +
+                    "expects more than %s missing intervals or increase " +
+                    "uncoveredIntervalsLimit configuration value\n"


Why is there a newline character here?

cdeszaq · 2017-05-03T16:31:27Z

fili-generic-example/src/main/resources/applicationConfig.properties

@@ -42,3 +42,6 @@ bard__top_n_enabled=true

 # Flag to turn on case sensitive keys in keyvalue store
 bard__case_sensitive_keys_enabled = false
+
+# Default value for Druid limit on uncovered interval
+bard__druid_uncovered_interval_limit = -1


This shouldn't be needed, the module config (where the default is specified) should be being used.

Still needs addressing I think?

cdeszaq · 2017-05-03T16:31:29Z

fili-core/src/test/resources/testApplicationConfig.properties

@@ -66,3 +66,6 @@ bard__intersection_reporting_enabled = false

 # Flag to turn on case sensitive keys in keyvalue store
 bard__case_sensitive_keys_enabled = false
+
+# Default value for Druid limit on uncovered interval
+bard__druid_uncovered_interval_limit = -1


This shouldn't be needed, the module config (where the default is specified) should be being used.

@cdeszaq @garyluoex I dropped it. But let me know if it needs to be kept and I can put it back at anytime.

cdeszaq · 2017-05-03T16:31:36Z

fili-wikipedia-example/src/main/resources/applicationConfig.properties

@@ -41,3 +41,6 @@ bard__top_n_enabled=true

 # Flag to turn on case sensitive keys in keyvalue store
 bard__case_sensitive_keys_enabled = false
+
+# Default value for Druid limit on uncovered interval
+bard__druid_uncovered_interval_limit = -1


This shouldn't be needed, the module config (where the default is specified) should be being used.

garyluoex

Need Change Log

garyluoex · 2017-05-04T19:00:45Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/query/QueryContext.java

@@ -36,7 +37,8 @@
        USE_CACHE("useCache"),
        POPULATE_CACHE("populateCache"),
        BY_SEGMENT("bySegment"),
-        FINALIZE("finalize");
+        FINALIZE("finalize"),
+        UNCOVERED_INTERVALS_LIMIT("uncoveredIntervalsLimit");


put the semicolon to the next line.

cdeszaq · 2017-05-05T16:37:35Z

fili-core/src/test/resources/testApplicationConfig.properties

@@ -63,6 +63,3 @@ bard__default_per_page = 10000

 # Intersection reporting enabled or not.
 bard__intersection_reporting_enabled = false
-
-# Flag to turn on case sensitive keys in keyvalue store
-bard__case_sensitive_keys_enabled = false


Why is this being removed here? Does this have anything to do with the rest of the changes in this PR?

cdeszaq · 2017-05-05T16:38:19Z

fili-generic-example/src/main/resources/applicationConfig.properties

@@ -42,3 +42,6 @@ bard__top_n_enabled=true

 # Flag to turn on case sensitive keys in keyvalue store
 bard__case_sensitive_keys_enabled = false
+
+# Default value for Druid limit on uncovered interval
+bard__druid_uncovered_interval_limit = -1


Still needs addressing I think?

garyluoex · 2017-05-05T17:26:00Z

CHANGELOG.md

@@ -9,6 +9,8 @@ Current
 -------
 ### Added:

+- [Prepare For Partial Data V2](https://github.com/yahoo/fili/pull/264)
+


list things like, added more error message, added new feature flag, added new queryContext

garyluoex

After change log update

michael-mclawhorn · 2017-05-05T20:20:38Z

fili-core/src/main/resources/moduleConfig.properties

@@ -124,6 +124,9 @@ bard__default_asyncAfter=never
 # Flag to turn on case sensitive keys in keyvalue store
 bard__case_sensitive_keys_enabled = false

+# Sets the upper limit of the number of Druid uncovered interval. Default is -1 meaning no uncovered interval is allowed
+bard__druid_uncovered_interval_limit = -1


Wouldn't 0 be just as good for 'no uncovered interval'?

@michael-mclawhorn @cdeszaq @garyluoex I vote for 0 but I'm open to either

Lets change it to 0, I was thinking something that doesn't work. Change all check for partial data v2 enabled to be strictly greater than 0 also.

@garyluoex @michael-mclawhorn I have changed it to 0. #267 has been update as well.

michael-mclawhorn · 2017-05-05T20:21:18Z

I'm a little unclear on what the value of a counter for number of uncovered intervals is. How is a count of intervals meaningful from a correctness perspective?

garyluoex · 2017-05-05T20:31:37Z

The count is required by druid to turn the feature on, at the same time limiting the total header size.

QubitPi added NEED 2 REVIEWS REVIEWABLE labels May 2, 2017

cdeszaq requested changes May 3, 2017

View reviewed changes

QubitPi added 2 commits May 3, 2017 12:51

Prepare For Partial Data V2

726d5d4

address @cdeszaq's comments

80325d6

QubitPi force-pushed the prepare-for-partial-data-v2 branch from 1cfd34f to 80325d6 Compare May 3, 2017 19:59

garyluoex self-assigned this May 4, 2017

garyluoex requested changes May 4, 2017

View reviewed changes

garyluoex added the NEED CHANGES label May 5, 2017

cdeszaq requested changes May 5, 2017

View reviewed changes

QubitPi added 2 commits May 5, 2017 10:14

address comments

a5e6c01

address comments

3135b53

garyluoex reviewed May 5, 2017

View reviewed changes

garyluoex approved these changes May 5, 2017

View reviewed changes

address comments

1408d64

QubitPi added NEED 1 REVIEW and removed NEED 2 REVIEWS NEED CHANGES labels May 5, 2017

michael-mclawhorn reviewed May 5, 2017

View reviewed changes

QubitPi added 2 commits May 5, 2017 13:41

address comments

e5de67a

fix comment error

e5f0c9e

cdeszaq approved these changes May 9, 2017

View reviewed changes

cdeszaq merged commit 8e3d7f4 into master May 9, 2017

cdeszaq deleted the prepare-for-partial-data-v2 branch May 10, 2017 18:55

QubitPi added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare For Partial Data V2 #264

Prepare For Partial Data V2 #264

QubitPi commented May 2, 2017 •

edited

Loading

cdeszaq May 3, 2017

cdeszaq May 3, 2017

cdeszaq May 3, 2017

cdeszaq May 5, 2017

cdeszaq May 3, 2017

QubitPi May 3, 2017

cdeszaq May 3, 2017

garyluoex left a comment

garyluoex May 4, 2017

cdeszaq May 5, 2017

cdeszaq May 5, 2017

garyluoex May 5, 2017

garyluoex left a comment

michael-mclawhorn May 5, 2017

QubitPi May 5, 2017

garyluoex May 5, 2017 •

edited

Loading

QubitPi May 5, 2017

michael-mclawhorn commented May 5, 2017

garyluoex commented May 5, 2017

Prepare For Partial Data V2 #264

Prepare For Partial Data V2 #264

Conversation

QubitPi commented May 2, 2017 • 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

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

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

garyluoex May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-mclawhorn commented May 5, 2017

garyluoex commented May 5, 2017

QubitPi commented May 2, 2017 •

edited

Loading

garyluoex May 5, 2017 •

edited

Loading