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

Rename filter variables and methods in DataApiRequest #507

Merged
merged 8 commits into from
Aug 25, 2017

Conversation

mpardesh
Copy link
Contributor

Addresses #497

@archolewa
Copy link
Contributor

archolewa commented Aug 17, 2017

Note that this is a breaking change (anybody who uses either of the getters in a request mapper will experience compile errors when they upgrade).

We generally try to avoid breaking changes, especially in small updates. Instead, we should add new methods with the desired names, and deprecate the old methods. Then we can remove the deprecated methods in a later major version rev, where breaking changes are more or less expected.

Also we should add a CHANGELOG.

Copy link
Contributor

@archolewa archolewa left a comment

Choose a reason for hiding this comment

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

Forgot to use the Request Changes feature, and I can't do a blank request changes.

return this.apiFilters;
}

@Deprecated
public Map<Dimension, Set<ApiFilter>> getFilters() {
Copy link
Contributor

Choose a reason for hiding this comment

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

These deprecated methods should delegate to the non-deprecated version (i.e. getFilters() should return getApiFilters(). Furthermore, we should add a Javadoc, with a @deprecated annotation indicating which method to use instead.

*
* @deprecated this method has been deprecated because its name is unclear. Please use {@link #getDruidFilter()}.
*/
@Deprecated
public Filter getFilter() {
try (TimedPhase timer = RequestLog.startTiming("BuildingDruidFilter")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just delegate to getDruidFilter.

@archolewa
Copy link
Contributor

@mpardesh Just reviewed this change. Had a few small comments. Once those are addressed, I'll be willing to give my approval.

@yahoo yahoo deleted a comment Aug 24, 2017
@michael-mclawhorn
Copy link
Contributor

There's a problem with the underlying issue.

There's a reason that API filters are considered plural while Druid filters are singular. Because API filters are treated as a list (or a list grouped into a map by dimension) while the Druid filters are a tree with a leaf node as the reference variable.

@yahoo yahoo deleted a comment Aug 25, 2017
@archolewa
Copy link
Contributor

archolewa commented Aug 25, 2017

@michael-mclawhorn As far as I can tell, the api filters method is still called getApiFilters, while the druid filter is called getDruidFilter. So things are still plural and singular appropriately.

@michael-mclawhorn
Copy link
Contributor

@archolewa Yeah, I see that.
I actually think I'd rather make a breaking change than each the deprecation cost here, but I suppose that's not the friendliest choice. This is going to create rebase pain with #501, so I'd merge it quickly if you don't want to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants