Dimension Field Tagging and Dynamic Dimension Field Serilization #137

garyluoex · 2017-01-14T00:05:10Z

Added a new module fili-navi for components added to support for Navi
Added TaggedDimensionField and related components in fili-navi
Changed fili-core dimension endpoint DimensionField serialization strategy from hard coded static attributes to dynamic serialization based on jackson serializer
Deprecated DimensionsServlet::getDimensionFieldListSummaryView and DimensionsServlet::getDimensionFieldSummaryView
since there is no need for it anymore due to the change in serialization of DimensionField

garyluoex · 2017-01-17T16:34:16Z

...vi/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/DefaultDimensionFieldTag.java

+     * Constructor, build name using camel cased enum name.
+     */
+    DefaultDimensionFieldTag() {
+        this.tagName = EnumUtils.camelCase(name());


CamelCase or just LowerCase? (Does LowerCase feel more like tags?)

A quick survery of online tag systems suggests that they are mostly flat cased and searched case insensitively. I agree that all-lower is the right impulse.

If we do make them sortable, I'd make DimensionTag comparable on getName()

michael-mclawhorn

I concur with making tags flat cased. I also think that they should be unique on a field (e.g. on any given field, there is not a multiset quality), and I think that having a canonical externally imposed order will make for more consistent behavior.

michael-mclawhorn · 2017-01-17T20:40:24Z

...vi/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/DefaultDimensionFieldTag.java

+     * Constructor, build name using camel cased enum name.
+     */
+    DefaultDimensionFieldTag() {
+        this.tagName = EnumUtils.camelCase(name());


A quick survery of online tag systems suggests that they are mostly flat cased and searched case insensitively. I agree that all-lower is the right impulse.

michael-mclawhorn · 2017-01-17T20:43:03Z

fili-navi/src/main/java/com/yahoo/bard/webservice/data/dimension/TaggedDimensionField.java

+     *
+     * @return a list of tags
+     */
+    List<? extends Tag> getTags();


I think tags should probably be a set. Maybe even a sorted set to encourage canonical display.

michael-mclawhorn · 2017-01-17T20:45:59Z

fili-navi/src/test/java/com/yahoo/bard/webservice/data/dimension/TestTaggedDimensionField.java

+    TEST_PRIMARY_KEY(Collections.singletonList(PRIMARY_KEY)),
+    TEST_DISPLAY_NAME(Arrays.asList(PRIMARY_KEY, PRIMARY_KEY)),
+    TEST_DESCRIPTION(Collections.<Tag>emptyList())
+    ;


If these fields exist only to illustrate behavior on single tag, multiple tags and no tags, you might want to just rename the dimensions to reflect that purpose.

Also, would it be clearer to instantiate these tags as singletons in the test class? That would make it easer to see what you're testing by putting it next to the test itself, if the test is the only place these are being consumed.

michael-mclawhorn · 2017-01-17T20:46:39Z

...vi/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/DefaultDimensionFieldTag.java

+     * Constructor, build name using camel cased enum name.
+     */
+    DefaultDimensionFieldTag() {
+        this.tagName = EnumUtils.camelCase(name());


If we do make them sortable, I'd make DimensionTag comparable on getName()

cdeszaq

Nothing major, just a few small things to tighten up.

cdeszaq · 2017-01-18T17:57:07Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/data/SerializationResources.groovy

@@ -334,6 +334,6 @@ class SerializationResources extends Specification {
    }

    String getSerializedReponseContext1() {
-        """["com.yahoo.bard.webservice.web.responseprocessors.ResponseContext",{"randomHeader":"someHeader","apiMetricColumnNames":["java.util.LinkedHashSet",["metric1, metric2"]],"requestedApiDimensionFields":["java.util.LinkedHashMap",{"ageBracket":["java.util.LinkedHashSet",[["com.yahoo.bard.webservice.data.dimension.BardDimensionField","ID"]]]}]}]"""
+        """["com.yahoo.bard.webservice.web.responseprocessors.ResponseContext",{"randomHeader":"someHeader","apiMetricColumnNames":["java.util.LinkedHashSet",["metric1, metric2"]],"requestedApiDimensionFields":["java.util.LinkedHashMap",{"ageBracket":["java.util.LinkedHashSet",[["com.yahoo.bard.webservice.data.dimension.BardDimensionField",{"name":"id","description":"Dimension ID"}]]]}]}]"""


We should make sure that this don't make anything that was serialized in the old way explode. I'm not 100% clear on what was going on with this before (@archolewa may have some sense of it, since i think it's tied to the async work?), but we want to ensure we're not killing existing serialized things.

It seems like it is a custom serialization done in PreResponseSerializationProxy, which is used by at least Digits by MobstorPreResponseStore bind in DigitsBinderFactory. Still not sure if the format of dimension field stored matters or not, can dig deeper or ask Digits people if necessary.

The piece I'm most concerned about is the deserialization of this thing. That's what usually breaks when serialization / deserialization don't line up. Since this capability is lightly (if ever) used over a duration that spans more than a single request, it's probably OK to set this asside.

cdeszaq · 2017-01-18T18:00:50Z

fili-navi/src/main/java/com/yahoo/bard/webservice/data/dimension/Tag.java

+import com.fasterxml.jackson.annotation.JsonValue;
+
+/**
+ * An interface for tag.


This isn't giving us any info that the interface signature doesn't already have. Instead, it would be good if this doc talked about what these tags are for (ie. indicating semantic meaning to dimension fields so the clients can have a better sense of properties of the fields). It would also be good to give some examples, like "primaryKey", "unique", "localId", etc.

I was thinking maybe one day we might need tag for other things, so this serves as a general purpose tag with a name interface. Let me know if you think we should indicate that it is for dimension field for now.

Since this is tucked under the dimension package, and we only have 1 use for tags at the moment, I think it's OK to keep it tight. If we need to make it more widely used later, that should be easy enough to do.

cdeszaq · 2017-01-18T18:01:49Z

fili-navi/src/main/java/com/yahoo/bard/webservice/data/dimension/TaggedDimensionField.java

+import java.util.Set;
+
+/**
+ * Dimension field with tags.


Again, a bit deeper on why tags are here and what purpose they have makes this doc more useful.

cdeszaq · 2017-01-18T18:02:12Z

fili-navi/src/main/java/com/yahoo/bard/webservice/data/dimension/TaggedDimensionField.java

+public interface TaggedDimensionField extends DimensionField {
+
+    /**
+     * Get a list of tags associated to the current field.


Need to update comment to reflect kind of collection, since it's no longer a list?

cdeszaq · 2017-01-18T18:42:51Z

...vi/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/DefaultDimensionFieldTag.java

+import java.util.Locale;
+
+/**
+ * Default dimension field tag provided in fili-navi to match fili-core concept "key" field.


I might drop the explicit mention of key fields at the class level, making this just be "... to match fili-core concepts". You could then move the info about "key field" onto JavaDoc on the PRIMARY_KEY instance.

cdeszaq · 2017-01-18T18:45:19Z

...vi/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/DefaultDimensionFieldTag.java

+     * Constructor, build name using camel cased enum name.
+     */
+    DefaultDimensionFieldTag() {
+        this.tagName = name().toLowerCase(Locale.ENGLISH);


Given that pretty much all other identifiers in the Fili ecosystem are camelCase, I think it might be a bit awkward for this to be small_snake_case. I know there's been some discussion on this already, and I agree that this shouldn't be PascalCase, but I don't think small_snake_case jives with the rest of the feel of Fili.

Sorry to push back and forth on this :)

Well not all, the physical names of things are all* lower cased, the logical names of things are all* camel cased. Let me know if you still want it to be camel case, thanks!

* Well, all that I know of

Right, I meant all the external names. Internal names, in general, do use snake_case, but external uses camelCase. (that's one distinction that actually helps identify where an identifier is coming from)

I'd say we should still go with camelCase for consistency

michael-mclawhorn · 2017-01-20T16:42:59Z

👍

garyluoex added NEED 2 REVIEWS REVIEWABLE labels Jan 14, 2017

garyluoex force-pushed the FieldTagging branch from 36e92c0 to 54acd7a Compare January 14, 2017 00:10

garyluoex commented Jan 17, 2017

View reviewed changes

michael-mclawhorn requested changes Jan 17, 2017

View reviewed changes

cdeszaq requested changes Jan 18, 2017

View reviewed changes

garyluoex added 5 commits January 18, 2017 15:38

Added fili-navi module with tagged dimension

abe8767

Change dimension field reponse serilization from static to dynamic

618c08d

Refined the test

da72b22

Modified according to mike's comments

2c95b82

Modified according to rick's comments

004d261

garyluoex force-pushed the FieldTagging branch from 2104fd0 to 004d261 Compare January 18, 2017 21:40

cdeszaq approved these changes Jan 18, 2017

View reviewed changes

michael-mclawhorn removed the NEED 2 REVIEWS label Jan 20, 2017

michael-mclawhorn approved these changes Jan 20, 2017

View reviewed changes

Added module setup test

9f27adf

garyluoex added MERGEABLE and removed REVIEWABLE labels Jan 20, 2017

michael-mclawhorn merged commit 2ae0605 into master Jan 20, 2017

garyluoex deleted the FieldTagging branch April 3, 2017 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dimension Field Tagging and Dynamic Dimension Field Serilization #137

Dimension Field Tagging and Dynamic Dimension Field Serilization #137

garyluoex commented Jan 14, 2017 •

edited

Loading

garyluoex Jan 17, 2017

michael-mclawhorn Jan 17, 2017

michael-mclawhorn Jan 17, 2017

michael-mclawhorn left a comment

michael-mclawhorn Jan 17, 2017

michael-mclawhorn Jan 17, 2017

michael-mclawhorn Jan 17, 2017

michael-mclawhorn Jan 17, 2017

cdeszaq left a comment

cdeszaq Jan 18, 2017

garyluoex Jan 18, 2017 •

edited

Loading

cdeszaq Jan 18, 2017

cdeszaq Jan 18, 2017

garyluoex Jan 18, 2017

cdeszaq Jan 18, 2017

cdeszaq Jan 18, 2017

cdeszaq Jan 18, 2017

cdeszaq Jan 18, 2017

cdeszaq Jan 18, 2017

garyluoex Jan 18, 2017 •

edited

Loading

cdeszaq Jan 18, 2017

michael-mclawhorn commented Jan 20, 2017

Dimension Field Tagging and Dynamic Dimension Field Serilization #137

Dimension Field Tagging and Dynamic Dimension Field Serilization #137

Conversation

garyluoex commented Jan 14, 2017 • edited Loading

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

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex Jan 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex Jan 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-mclawhorn commented Jan 20, 2017

garyluoex commented Jan 14, 2017 •

edited

Loading

garyluoex Jan 18, 2017 •

edited

Loading

garyluoex Jan 18, 2017 •

edited

Loading