Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix groupBy caching to work with renamed aggregators #1499

Merged
merged 1 commit into from
Jul 9, 2015

Conversation

nishantmonu51
Copy link
Member

Issue - while storing results in cache we store the event map which
contains aggregator names mapped to values. Now when someone fire same
query after renaming aggs, the cache key will be same but the event
will contain metric values mapped to older names which leads to wrong
results.

Fix - modify cache to not store raw event but the actual list of values
only.

@xvrl xvrl modified the milestone: 0.8.0 Jul 7, 2015
.setQuerySegmentSpec(SEG_SPEC)
.setDimFilter(DIM_FILTER)
.setGranularity(GRANULARITY)
.setDimensions(Arrays.<DimensionSpec>asList(new DefaultDimensionSpec("a", "a")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we test with a different output name as well? It's unclear which one should be stored in the cache

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added test.

@@ -444,6 +448,7 @@ public String apply(DimensionSpec input)
+ limitBytes.length
)
.put(GROUPBY_QUERY)
.put(CACHE_STRATEGY_VER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we spell out version instead of using abbreviations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@fjy
Copy link
Contributor

fjy commented Jul 9, 2015

👍

Issue - while storing results in cache we store the event map which
contains aggregator names mapped to values. Now when someone fire same
query after renaming aggs, the cache key will be same but the event
will contain metric values mapped to older names which leads to wrong
results.
Fix - modify cache to not store raw event but the actual list of values
only.

review comments + fix dimension renaming

review comment
xvrl added a commit that referenced this pull request Jul 9, 2015
fix groupBy caching to work with renamed aggregators
@xvrl xvrl merged commit b3d360f into apache:master Jul 9, 2015
@xvrl xvrl deleted the fix-groupby-caching branch July 9, 2015 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants