You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by dkhwangbo <gi...@git.apache.org> on 2018/05/14 08:32:06 UTC
[GitHub] nifi pull request #2696: NIFI-5188: DruidTranquilityController does not full...
GitHub user dkhwangbo opened a pull request:
https://github.com/apache/nifi/pull/2696
NIFI-5188: DruidTranquilityController does not fully support Druid aggregator
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [x] Is your initial contribution a single, squashed commit?
### For code changes:
- [x] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- I think verify this PR is enough with unit test already used.
- [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dkhwangbo/nifi NIFI-5188
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2696.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2696
----
commit b1080be5469b6e1011b560070c77201441689415
Author: Dongkyu Hwangbo <hw...@...>
Date: 2018-05-14T07:33:49Z
NIFI-5188: DruidTranquilityController does not fully support Druid aggregator
----
---
[GitHub] nifi pull request #2696: NIFI-5188: DruidTranquilityController does not full...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2696
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2696
@vakshorton do you mind taking a look at the changes?
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2696
+1 LGTM, I was able to verify that the existing aggregators are still present, and that the sketch and histogram aggregator modules are added. Thanks for this improvement! Merging to master
---
[GitHub] nifi pull request #2696: NIFI-5188: DruidTranquilityController does not full...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2696#discussion_r195729409
--- Diff: nifi-nar-bundles/nifi-druid-bundle/nifi-druid-controller-service/src/main/java/org/apache/nifi/controller/druid/DruidTranquilityController.java ---
@@ -518,56 +515,23 @@ public String getTransitUri() {
}
private List<AggregatorFactory> getAggregatorList(String aggregatorJSON) {
- List<AggregatorFactory> aggregatorList = new LinkedList<>();
- List<Map<String, String>> aggregatorInfo = parseJsonString(aggregatorJSON);
- for (Map<String, String> aggregator : aggregatorInfo) {
-
- if (aggregator.get("type").equalsIgnoreCase("count")) {
- aggregatorList.add(getCountAggregator(aggregator));
- } else if (aggregator.get("type").equalsIgnoreCase("doublesum")) {
- aggregatorList.add(getDoubleSumAggregator(aggregator));
- } else if (aggregator.get("type").equalsIgnoreCase("doublemax")) {
- aggregatorList.add(getDoubleMaxAggregator(aggregator));
- } else if (aggregator.get("type").equalsIgnoreCase("doublemin")) {
- aggregatorList.add(getDoubleMinAggregator(aggregator));
- } else if (aggregator.get("type").equalsIgnoreCase("longsum")) {
- aggregatorList.add(getLongSumAggregator(aggregator));
- } else if (aggregator.get("type").equalsIgnoreCase("longmax")) {
- aggregatorList.add(getLongMaxAggregator(aggregator));
- } else if (aggregator.get("type").equalsIgnoreCase("longmin")) {
- aggregatorList.add(getLongMinAggregator(aggregator));
- }
- }
-
- return aggregatorList;
- }
-
- private AggregatorFactory getLongMinAggregator(Map<String, String> map) {
- return new LongMinAggregatorFactory(map.get("name"), map.get("fieldName"));
- }
-
- private AggregatorFactory getLongMaxAggregator(Map<String, String> map) {
- return new LongMaxAggregatorFactory(map.get("name"), map.get("fieldName"));
- }
-
- private AggregatorFactory getLongSumAggregator(Map<String, String> map) {
- return new LongSumAggregatorFactory(map.get("name"), map.get("fieldName"));
- }
-
- private AggregatorFactory getDoubleMinAggregator(Map<String, String> map) {
- return new DoubleMinAggregatorFactory(map.get("name"), map.get("fieldName"));
- }
-
- private AggregatorFactory getDoubleMaxAggregator(Map<String, String> map) {
- return new DoubleMaxAggregatorFactory(map.get("name"), map.get("fieldName"));
- }
-
- private AggregatorFactory getDoubleSumAggregator(Map<String, String> map) {
- return new DoubleSumAggregatorFactory(map.get("name"), map.get("fieldName"));
- }
+ ComponentLog log = getLogger();
+ ObjectMapper mapper = new ObjectMapper(null);
+ mapper.registerModule(new AggregatorsModule());
+ mapper.registerModules(Lists.newArrayList(new SketchModule().getJacksonModules()));
+ mapper.registerModules(Lists.newArrayList(new ApproximateHistogramDruidModule().getJacksonModules()));
- private AggregatorFactory getCountAggregator(Map<String, String> map) {
- return new CountAggregatorFactory(map.get("name"));
+ try {
+ return mapper.readValue(
+ aggregatorJSON,
+ new TypeReference<List<AggregatorFactory>>()
+ {
--- End diff --
CheckStyle error here, the curly brace should be on the line above. I'll fix this on merge
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on the issue:
https://github.com/apache/nifi/pull/2696
@mattyb149 Manual modification is only way to update LICENSE files? Any tools or scripts to do with?
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on the issue:
https://github.com/apache/nifi/pull/2696
@joewitt @pvillard31 @mattyb149 Any ideas?
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2696
No scripts that I know of, since we can't count on where the sources are. Sorry for any confusion though, I was saying the L&N is good to go here, the additional transitive dependencies are accounted for. The code looks good to me (and a colleague), I just wanted to leave that note for folks looking at the part of the PR that added dependencies as we have to keep a close eye on what's included in the distribution.
I will test the aggregators on a live system ASAP and hopefully get this merged soon, thanks again!
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2696
Verified L&N so far, the only unaccounted for dependency is Yahoo's sketches-core, which is ASL 2.0 licensed with no NOTICE: https://github.com/DataSketches/sketches-core
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by mattyb149 <gi...@git.apache.org>.
Github user mattyb149 commented on the issue:
https://github.com/apache/nifi/pull/2696
The code changes LGTM, but I haven't built/tried them yet. I see quite a few [changes in 0.9.2](https://github.com/druid-io/druid/pulls?utf8=%E2%9C%93&q=is%3Apr%20is%3Aclosed%20milestone%3A0.9.2), do any of them not work with our current version of Tranquility?
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on the issue:
https://github.com/apache/nifi/pull/2696
@joewitt @pvillard31 @mattyb149 Any ideas?
---
[GitHub] nifi issue #2696: NIFI-5188: DruidTranquilityController does not fully suppo...
Posted by dkhwangbo <gi...@git.apache.org>.
Github user dkhwangbo commented on the issue:
https://github.com/apache/nifi/pull/2696
@mattyb149 Hi. Thanks for your comment.
I already use this patch builded with Druid 0.9.2 for a week and no abnormal operation is detected. But I think Using Druid 0.9.1 is appropriate way because latest version of tranquility is builded against Druid 0.9.1(refer: [tranquility 0.8.2 release note](https://github.com/druid-io/tranquility/releases/tag/v0.8.2)).
I rollback this change. Please review.
---