Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

In processAllUnacks, change it so only the unacks from the past get reset #1875

Merged
merged 15 commits into from
Oct 13, 2020

Conversation

rickfish
Copy link
Contributor

This is a solution for Issue #1859. Tasks that were popped had their popped status set to false before they were acked so a future poll could get the same task. This PR only resets the popped flag for tasks that were popped in the past by a minute, not in the future by a minute.

@deluxor
Copy link

deluxor commented Sep 18, 2020

Let me vote "Merge" on this one...
Its being used in our test env for the past 3 days without issues.

@apanicker-nflx
Copy link
Collaborator

Please fix the build failure :

com.netflix.conductor.dao.mysql.MySQLQueueDAOTest > processUnacksTest FAILED
    java.lang.AssertionError at MySQLQueueDAOTest.java:304

@codecov
Copy link

codecov bot commented Sep 24, 2020

Codecov Report

Merging #1875 into dev will increase coverage by 0.08%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #1875      +/-   ##
============================================
+ Coverage     64.91%   65.00%   +0.08%     
- Complexity     3866     3888      +22     
============================================
  Files           298      299       +1     
  Lines         18387    18451      +64     
  Branches       1674     1684      +10     
============================================
+ Hits          11936    11994      +58     
- Misses         5623     5624       +1     
- Partials        828      833       +5     
Impacted Files Coverage Δ Complexity Δ
...lix/conductor/core/execution/WorkflowExecutor.java 78.22% <83.33%> (+0.44%) 183.00 <1.00> (+3.00)
...com/netflix/conductor/dao/mysql/MySQLQueueDAO.java 85.24% <100.00%> (ø) 49.00 <1.00> (ø)
...tflix/conductor/dao/postgres/PostgresQueueDAO.java 90.90% <100.00%> (+3.30%) 52.00 <1.00> (+1.00)
...flix/conductor/server/resources/AdminResource.java 85.71% <0.00%> (-14.29%) 4.00% <0.00%> (ø%)
...flix/conductor/core/execution/WorkflowSweeper.java 72.88% <0.00%> (-5.70%) 10.00% <0.00%> (+1.00%) ⬇️
...onductor/dao/es5/index/ElasticSearchRestDAOV5.java 73.60% <0.00%> (-0.68%) 57.00% <0.00%> (ø%)
...om/netflix/conductor/service/AdminServiceImpl.java 3.44% <0.00%> (-0.26%) 1.00% <0.00%> (ø%)
...x/conductor/client/automator/TaskPollExecutor.java 74.80% <0.00%> (-0.20%) 16.00% <0.00%> (ø%)
...onductor/core/execution/WorkflowRepairService.java 71.42% <0.00%> (ø) 9.00% <0.00%> (?%)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 749f922...2f25c79. Read the comment docs.

@rickfish
Copy link
Contributor Author

@apanicker-nflx I fixed the build error but I did it by converting the test case so that it would work with the new code. I am not completely sure whether the old test case was constructed to conform to the old code or if it was a legitimate test case that I should have left alone. I would like someone else to review it if possible.

@apanicker-nflx
Copy link
Collaborator

@mactaggart @mashurex Could you please help with reviewing this pull request? Thanks

Copy link
Contributor

@mashurex mashurex left a comment

Choose a reason for hiding this comment

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

The changes for mysql look good.

Copy link
Contributor

@mactaggart mactaggart left a comment

Choose a reason for hiding this comment

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

changes look good. Thanks!

@apanicker-nflx apanicker-nflx merged commit e8128f0 into Netflix:dev Oct 13, 2020
TwoUnderscorez pushed a commit to TwoUnderscorez/conductor that referenced this pull request Jul 23, 2021
…eset (Netflix#1875)

* only reset unacks from the past

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

* fix the test cases so that it matches the change to the code

Co-authored-by: u447 <rick.fishman@bcbsfl.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants