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

FEATURE: Persist unpersisted changes in CLI after messageFinished signal #45

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sorenmalling
Copy link

Once a messageFinished signal is emitted, the Persistence Manager persist all unpersisted changes. This is to have the jobmanager act as a normal CLI command, known from the a CommandController command

Resolves #44

…gnal

Once a `messageFinished` signal is emitted, the Persistence Manager persist all unpersisted changes. This is to have the jobmanager act as a normal CLI command, known from the a CommandController command
Copy link

@albe albe left a comment

Choose a reason for hiding this comment

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

Yeah, sure, why not? @bwaidelich wdyt?

@albe albe changed the title [FEATURE] Persist unpersisted changes in CLI after messageFinished signal FEATURE: Persist unpersisted changes in CLI after messageFinished signal Jan 28, 2020
Copy link
Contributor

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

I shared my concerns here: #44 (comment)

@bwaidelich bwaidelich dismissed their stale review January 29, 2020 08:45

I'll remove my -1 for now because I don't want to block this, but: #44 (comment)

@albe
Copy link

albe commented Jan 29, 2020

Yeah, I guess you are not wrong in that regard. The jobqueue should be reliable about the processing of messages, and a message is processed after it finishes doing what it does, including persisting data.
If you depend on persistAll() after processing, you effectively opt into "at most once" processing, while you probably do want "at least once" in pretty much all cases.

We should document that better and maybe create an easy way to conciously opt into this? i.e. toggle that automatic "persistAll" via a setting?

Edit: See my comment in #44 (comment) - I do think we should bring the auto-persistence in line between isolated and non-isolated execution, but probably messageFinished isn't the right place.

Copy link
Member

@markusguenther markusguenther left a comment

Choose a reason for hiding this comment

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

Thanks again for today to @sorenmalling :)
For me that look pretty solid.

@sorenmalling
Copy link
Author

I'm in favor of running everything isolated instead. That will call the persistence manager as normal cli and not produce any unwanted issues from calling the persistenceManager

@albe
Copy link

albe commented Feb 27, 2020

A good place would be between the executeJobForMessage and the queue->finish(...) call - or to make it behave same for "isolated" and "non-isolated", add a signal at the end of executeJobForMessage and do a persist call there?

Anyone have some thoughts about my suggestion in the issue? I still think that would be the better place to hook the persist call into, because otherwise you have "at most once" behaviour (work is done, message marked finished, but the persist call might still fail). It's also the same semantic as with the executeIsolated case - since the persist call happens in the subprocess, but the message is "finished" only afterwards.

I'd maybe call that signal JobExecuted and put it at the end of the executeJobForMessage() method.

@sorenmalling
Copy link
Author

@albe I don't know which is better :-)

My sole argument for running isolated, is that the SQL for updating the message queue (it's using the connection directly) is that, if the persistence fails, so does the updating of the jobqueue. So we will not end up with "completed" jobs, where persistence failed.

I don't mind what proposal goes on - I just want users of the package not to have different experiences depending on the configuration setting :-)

@albe
Copy link

albe commented Mar 15, 2020

I don't mind what proposal goes on - I just want users of the package not to have different experiences depending on the configuration setting

Absolutely, that's exactly why I suggest the JobExecuted signal and put the persistAll() there

@bwaidelich bwaidelich removed their request for review December 7, 2020 16:15
@kdambekalns
Copy link
Member

Another old one that need a decision, it seems… do you people still remember your thoughts? 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Queue jobs executed in CLI does not persist repository changes unless executed isolated
5 participants