[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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...

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...

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...

Github user mgaido91 commented on the issue: https://github.com/apache/nifi/pull/2657 LGTM, thanks @viazovskyi. @ijokarumawak @joewitt any other comments? ---

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

Github user viazovskyi commented on the issue: https://github.com/apache/nifi/pull/2657 I think now PR looks better ---

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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 wrote: > @viazovskyi I suggest

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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...

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 wrote: >

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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 issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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...

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...

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...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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 issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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...

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...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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.

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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

[GitHub] nifi issue #2657: NIFI-5109 Reset justElectedPrimaryNode flag right after re...

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? ---