You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by viazovskyi <gi...@git.apache.org> on 2018/04/25 15:16:09 UTC
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
GitHub user viazovskyi opened a pull request:
https://github.com/apache/nifi/pull/2657
NIFI-5109 Reset justElectedPrimaryNode flag right after reelection ha…
…ppen
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?
- [ ] 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/viazovskyi/nifi nifi-5109
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2657.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 #2657
----
commit ed1ef083e1373b42f1301f0faff9105018c03135
Author: Max Viazovskyi <il...@...>
Date: 2018-04-25T14:48:43Z
NIFI-2109 Reset justElectedPrimaryNode flag right after reelection happen
----
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi I would need to evaluate more thoroughly but I believe this change has a check and reset concurrent issue. While you're altering a volatile field (so the change will be consistent across threads in reading) what is not guaranteed is the timing of when the check happens versus the reset. This might need an alternative approach or a lock.
cc @markap14
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r189462936
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -375,7 +376,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
--- End diff --
no I just meant moving this line `context.yield();` in the `if` you introduced, before the return statement.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
@bbende @ijokarumawak @mcgilman Guys, would you like to perform code review?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
I agree, it's possible. Then probably place flag reset after the getting the state, but before checking what we have read?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
I think now PR looks better
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r189461079
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -375,7 +376,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
--- End diff --
Do you mean move the whole if and do not use yielded variable?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
yep, i see, i'm new for git, could you please guide how can i rollback this
merge?
On Mon, May 21, 2018 at 1:37 PM Marco Gaido <no...@github.com>
wrote:
> @viazovskyi <https://github.com/viazovskyi> your last merge screwed a bit
> this PR. May you please restore it? Thanks.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/nifi/pull/2657#issuecomment-390618294>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ABeUEgua6ELz2o1UHFyYqNoTPXi5nNZPks5t0pjlgaJpZM4Tjoic>
> .
>
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2657
I also agree on @ijokarumawak's suggestion, may you please update the PR accordingly @viazovskyi ?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
@joewitt Hi Joe, is there any update?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi I suggest you something like:
```
git checkout nifi/master
git checkout -b nifi-5109_2
// do your changes on this branch and commit them
git branch -D nifi-5109
git branch -m nifi-5109
git push -f -u origin nifi-5109
```
If you want details/explanation on each command let me know. Or let me know if I can help you somehow. Thanks.
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2657
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r188866746
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -375,7 +375,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
- return;
+ break;
--- End diff --
sorry, I just realize that now, with this fix we are performing an extra listing which is not needed. Maybe it is better to just have return as before, but prepend a `justElectedPrimaryNode = false;`. So I am thinking of substituting this line with:
```
justElectedPrimaryNode = false;
return;
```
what do you think @ijokarumawak @joewitt @viazovskyi ?
Thanks
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi Ok fair enough. So between that and the other problem which is you could fail to properly handle the case when it does become primary i think we still have some logic work to do.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
Thanks Joe. There are more volatile fields (lastListedLatestEntryTimestampMillis, lastProcessedLatestEntryTimestampMillis, etc) altered after the check. So this should be redesigned what do you think?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
@mgaido91 I dont have any other comments for now - i'm looking into some other things so if ya'll are good then I think we're set.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
hi @ijokarumawak, it looks good to me, @joewitt what do you think?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
@joewitt if i put it closer to the catch i'm afraid the flag will not be reset due to [line 377](https://github.com/apache/nifi/blob/rel/nifi-1.6.0/nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java#L377) where onTrigger method hits the context.yield() and returns if nothing was changed from the previous run. That was the initial idea to reset it as soon as possible.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
With the location of the reset now being where it is placed what happens in the event that the pulling of state from the state manager results in an exception? My read is that it would mean we'd not try again to pull the state later because we cleared the flag but also we'd not have the latest state.
Do you not believe that is possible?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi i think it is important to address that issue (ensuring we dont reset the state check until we know we've pulled latest state if available) before we move forward.
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r189455719
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -356,6 +356,7 @@ private EntityListing deserialize(final String serializedState) throws JsonParse
@Override
public void onTrigger(final ProcessContext context, final ProcessSession session) throws ProcessException {
Long minTimestampToListMillis = lastListedLatestEntryTimestampMillis;
+ boolean yielded = false;
--- End diff --
nit: I'd propose a more meaningful name, like `isUpToDate` or something similar.
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r189470396
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -375,7 +376,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
--- End diff --
ok, never mind. I'll give variable new name and move context.yield()
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi do you think it is possible to add a UT for this?
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r189195817
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -375,7 +375,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
- return;
+ break;
--- End diff --
@mgaido91 Good catch, I was only looking at the method partially.. Thanks, that makes sense to me.
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r189464759
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -375,7 +376,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
--- End diff --
I'd like to move the original 'if' with yield out of the loop closer catch, then I don't need this new 'if' and variable. What do you think?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
Thanks Marco, I'll try it a bil later
On Mon, May 21, 2018 at 5:03 PM Marco Gaido <no...@github.com>
wrote:
> @viazovskyi <https://github.com/viazovskyi> I suggest you something like:
>
> git checkout nifi/master
> git checkout -b nifi-5109_2
> // do your changes on this branch and commit them
> git branch -D nifi-5109
> git branch -m nifi-5109
> git push -f -u origin nifi-5109
>
> If you want details/explanation on each command let me know. Or let me
> know if I can help you somehow. Thanks.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/nifi/pull/2657#issuecomment-390662714>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ABeUEhO7XujAV_vWeO_eDNpJjA9egoEUks5t0skugaJpZM4Tjoic>
> .
>
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi your last merge screwed a bit this PR. May you please restore it? Thanks.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi I think that the easiest way is probably creating a new branch locally from the master, reapply your changes on it and force push to this branch.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/nifi/pull/2657
LGTM, thanks @viazovskyi.
@ijokarumawak @joewitt any other comments?
---
[GitHub] nifi pull request #2657: NIFI-5109 Reset justElectedPrimaryNode flag right a...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2657#discussion_r189455715
--- Diff: nifi-nar-bundles/nifi-extension-utils/nifi-processor-utils/src/main/java/org/apache/nifi/processor/util/list/AbstractListProcessor.java ---
@@ -375,7 +376,7 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
--- End diff --
nit: I'd prefer if we move this before the return statement
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi LGTM, +1. Thanks for finding this, providing the detailed steps to produce the issue and fixing it! The newly added unit test clarifies how it should behave. Merging in.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
fields being volatile can be fine. volatile makes a more explicit assertion about how java will guarantee the value to be read across threads. The issue, at least from a quick look, is the potential race condition between when it is checked versus when it is set. I belive it looked like more than ine thread could get through the check.
It needs to be looked into more and if necessary just protected for the whole check/set operation to be guarded by a lock.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
@mgaido91 Adding UT I realized that just reset the flag and then return is not enough, because cluster state is not fully read (e.g. latestIdentifiersProcessed was not getting initialized) if i do return after yield. Please review the new commit after all checks completed. The idea is give the loop a chance to iterate over all read cluster' state and if processor should yield then exit the method after resetting the flag.
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by ijokarumawak <gi...@git.apache.org>.
Github user ijokarumawak commented on the issue:
https://github.com/apache/nifi/pull/2657
Maybe I'm thinking the issue oversimplified, but changing the line 378 `return` to `break` would address the issue while ensuring that we don't reset the flag until we know we've pulled the latest state, wouldn't it?
```
if (LATEST_LISTED_ENTRY_TIMESTAMP_KEY.equals(k)) {
minTimestampToListMillis = Long.parseLong(v);
// If our determined timestamp is the same as that of our last listing, skip this execution as there are no updates
if (minTimestampToListMillis.equals(this.lastListedLatestEntryTimestampMillis)) {
context.yield();
return; // How about changing this to 'break'?
} else {
this.lastListedLatestEntryTimestampMillis = minTimestampToListMillis;
}
} else if (LAST_PROCESSED_LATEST_ENTRY_TIMESTAMP_KEY.equals(k)) {
this.lastProcessedLatestEntryTimestampMillis = Long.parseLong(v);
} else if (k.startsWith(IDENTIFIER_PREFIX)) {
latestIdentifiersProcessed.add(v);
}
```
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
done
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
@viazovskyi I just noticed this processor is trigger serially which means it isn't designed for concurrent use and in nifi will only ever have one thread. So we can relax on the check/modify piece. I think you want the flag reset lower in the block (end of try inside try/catch closer to where it was).
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the issue:
https://github.com/apache/nifi/pull/2657
@ijokarumawak can you take a look and merge if seems ok?
---
[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...
Posted by viazovskyi <gi...@git.apache.org>.
Github user viazovskyi commented on the issue:
https://github.com/apache/nifi/pull/2657
@joewitt not sure what is another problem you're talking about, the original [issue](https://issues.apache.org/jira/browse/NIFI-5109) is about onTrigger method never goes further line 377 and stops calling performListing after reelection of primary happened and same node became the primary again, that's why I reset the flag and it makes sense to me, because the election finished and further logic should only depend on another 2 fields lastListedLatestEntryTimestampMillis, lastProcessedLatestEntryTimestampMillis which are initialized from the state manager. But if performListing is not calling anymore then no any updates for the state and these 2 fields keep the same values. huh, hope it shows the issue in details :) @joewitt what is another issue?
---