NIFI-6822: Ensure that when we manage a Map of ID -> Count, that we p…#3853
NIFI-6822: Ensure that when we manage a Map of ID -> Count, that we p…#3853markap14 merged 1 commit intoapache:masterfrom
Conversation
| combined.putAll(first); | ||
| combined.putAll(second); | ||
| final Map<String, Long> combined = new HashMap<>(first); | ||
| second.forEach((key, value) -> combined.merge(key, value, (v1, v2) -> v1 + v2)); |
There was a problem hiding this comment.
Could this not be expressed a little more succinctly as second.forEach(combined.putIfAbsent) ? Same relative outcome no? I am glancing quickly so ignore me if I'm crazy.
There was a problem hiding this comment.
I don't believe so. putIfAbsent would say "If not there, add it. If there, leave the destination it alone." merge says "If not there, add it. If there, combine the value from both maps." So the result is different if both maps have the key.
There was a problem hiding this comment.
In retrospect after a nice refreshing break I'm not sure how I arrived at such a question.
| } | ||
|
|
||
| private <K, V> void mergeMaps(final Map<K, V> destination, final Map<K, V> toMerge, final BiFunction<? super V, ? super V, ? extends V> merger) { | ||
| if (toMerge == null) { |
There was a problem hiding this comment.
As a safety could you add a null check for destination too?
There was a problem hiding this comment.
not sure that null check against destination makes sense in the way this is being used. If toMerge is null, we can simply do nothing, because there is nothing to merge. But if the destination is null, we should not return without merging the values that are expected to be merged. It makes more sense IMO to throw a NPE in such a case. Is also worth noting that currently, all objects that could be passed in for destination are guaranteed non-null and if that were to change, the existing unit tests would fail.
| final V value = entry.getValue(); | ||
|
|
||
| final V destinationValue = destination.get(key); | ||
| if (destinationValue == null) { |
There was a problem hiding this comment.
Could this simply be a foreach -> merge with a biconsumer accepting the original bi consumer? Might shorten the code..but again my ten thousand foot overview look might be totally off base.
phrocker
left a comment
phrocker
left a comment
There was a problem hiding this comment.
I think the changes are good at a high level. I won't merge and leave it up to you whether my comments are something to address. Happy to merge if you feel they are non issues.
…roperly merge those maps during a checkpoint instead of overwriting existing values
|
Rebased against master. Thanks for reviewing & approving @phrocker |
2 participants
…roperly merge those maps during a checkpoint instead of overwriting existing values
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
Description of PR
Enables X functionality; fixes bug NIFI-YYYY.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
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.
Has your PR been rebased against the latest commit within the target branch (typically
master)?Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
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.