Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue974 yet another scheduled_poll refactor PR #991

Merged
merged 25 commits into from
Apr 2, 2024
Merged

Conversation

petersilva
Copy link
Contributor

@petersilva petersilva commented Mar 21, 2024

closes #974

This is a third PR ( of the poll refactor work, replacing #985 (which in turn replaced another one) which had five separate commits, that individually do not pass the flow tests, so would break git bisection. Want to keep bisection working, so re-created the branch with those five commits squashed together.

Once that was done, added a refactor of the Flow class's (sarracenia.flow.Flow) run() routine. It was very long (288 lines)

  • made called routines for flow algorithm phases: gather, filter, work, post routines more responsible (moved code into them.)
  • the phase routines now call the relevant plugin entry points
  • the phase routines now re-arrange the worklists.

Once the run() routine was shortened (to 155 lines... better, but not really good... enough to understand the algorithm) it felt safe to re-arrange algorithmic flow so that a "poll" can now download, identically to other components. (the work phase used to not be run when running a poll.)

comments from original PR (with annotations for what was tackled):

  • It doesn't do a poll on startup, currently. it waits for one interval before the first poll. easy to fix... should we?
    (FIXED by: 3e4ba34 , adjusted by: 06fa2d3 )

So now in poll, the loop towards the end of the run() routine is only used to sleep for 1 second at a time... and the real waiting is done in the scheduled.poll plugin instead.

  • In watch... still using long sleeps in that loop at the end... Is this an opportunity to simplify sr_watch also... so it also uses the same methods? Is there an advantage to doing it that way?

  • I'm wondering if there is a way to have watch use a shared duplicate suppression cache the same way as poll does,
    populating itself with the output exchange binding queue. Same benefit as poll, run on two machines with a vip and
    failover. (not done...)

  • wondering if this change might be an opportunity to simplify sarracenia.flow/init.py run() routine. It feels too complicated.
    (DONE 71fe41d ac9b102 32b2554 06fa2d3 52219cd ) Now poll runs work() aka do() so if "directory" were normal... it could be used by poll, see other PR Issue986 - stop converting *directory* to *path* in poll. #987

@petersilva
Copy link
Contributor Author

this one should not be squashed... just merged in...

@petersilva petersilva changed the title Issue974 yet another poll refactor PR Issue974 yet another scheduled_poll refactor PR Mar 21, 2024
@petersilva
Copy link
Contributor Author

OK this looks ok now, a merge should be clean.

There is a problem I ran into where options not declared in the main routines don't get parsed with the right types (opened as #992) there is a fix in this PR for that. Had the same problem working with @mshak2
this week where a add_option thing (a flag in that case) had the wrong type.

It was needed because scheduled_interval was such a value, and it was getting a string value, and ... silly.
so now, when converting a poll from v2 to sr3 the sleep setting is converted as scheduled_interval.
the defaulting still happens, but only if none of the scheduled* options are set.

all the commits are in an order that supports clean git bisection.

@petersilva
Copy link
Contributor Author

oh... and for testing purposes the branch to look at is https://github.com/MetPX/sarracenia/tree/issue974

@reidsunderland
Copy link
Member

It looks like the option parsing change broke the logEvents setting in the flow test?

@petersilva
Copy link
Contributor Author

It looks like the option parsing change broke the logEvents setting in the flow test?

it's fixed already. ( 7ed9557 )

I did a forced update to back out a commit, then re-ordered things so the tests never fail.

@petersilva
Copy link
Contributor Author

found some more issues with set parsing working with Mathew.
more patches inbound.

@petersilva
Copy link
Contributor Author

@reidsunderland we looked at all the commits together (with @andreleblanc11 ) and we accepted all the other PR's.
merged those into this branch. So it should be really good to do... Would appreciate a last round of testing before we merge it back into development.

Thanks.

@petersilva
Copy link
Contributor Author

OK looking ok after the weekend. after discussion, merging.

@petersilva petersilva merged commit 13a4416 into development Apr 2, 2024
2 of 4 checks passed
@petersilva petersilva deleted the issue974 branch May 8, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants