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

feat: Add DeleteEntitiesIter for viur-transactionmarker #1440

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

Conversation

ArneGudermann
Copy link
Contributor

No description provided.

@ArneGudermann ArneGudermann added feature New feature or request Priority: Low This issue can be considered with enough idle time. labels Mar 13, 2025
Comment on lines +880 to +881
query = db.Query("viur-transactionmarker").filter("creationdate <",
datetime.datetime.now() - datetime.timedelta(seconds=270))
Copy link
Member

Choose a reason for hiding this comment

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

This time is too short.

If you delete this before the task run the task would be abortet.

marker = db.Get(db.Key("viur-transactionmarker", env["transactionMarker"]))
if not marker:
logging.info(f"""Dropping task, transaction {env["transactionMarker"]} did not apply""")
return

We need a delta of a month or something like that because you can delay tasks in the future and tasks may fail a few times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I set this to one month

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay the problem is the max countdown for a Transaction is 270 seconds and only 90 in Idle so we can delete this after 270 seconds imo.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @ArneGudermann, can you comment this thing in the source-code? A reference to this issue would also be well.

Copy link
Member

@sveneberth sveneberth Mar 15, 2025

Choose a reason for hiding this comment

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

Yes, the transaction times are correct. But they are totally irrelevant for the transaction marker.

The reason is that a deferred task was called within a transaction:

if db.IsInTransaction():
# We have to ensure transaction guarantees for that task also
env["transactionMarker"] = db.acquireTransactionSuccessMarker()
# We move that task at least 90 seconds into the future so the transaction has time to settle
_countdown = max(90, _countdown) # Countdown can be set to None

And the tasks should only run if the transaction was successful and that is when the marker entity exists. If the transaction fails, the marker is also removed in the rollback.
AFTER the transaction has been successful, this marker object must now exist at least until the task has been fully executed. And when the task is executed can be at the earliest 90 seconds after the call, but also any time in the future. The task can be called with an eta for the next month or a countdown of 90 hours. But the task queue could also be so full that the task is simply only executed after thousands of others. There is no fixed time here. You can't predict that. Actually, the transaction marker can therefore only be safely removed after the task has been fully executed and not at any fixed date. In reality, however, tasks are completed within 30 days of the call deferred call and anything after that can be considered a corpse IMO.

If you remove the markers BEFORE the task has been executed, this would lead to the assumption that the transaction was not successful. Which in turn means that the task is also not executed:

if "transactionMarker" in env:
marker = db.Get(db.Key("viur-transactionmarker", env["transactionMarker"]))
if not marker:
logging.info(f"""Dropping task, transaction {env["transactionMarker"]} did not apply""")
return

But that would be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I think I missunderstood this Feature im the Frist Place. Thank you for the explaing. We can set the time to one month or we can create a conf variable for that. Or we can set a success to the Marker after the transaction.

Comment on lines +880 to +881
query = db.Query("viur-transactionmarker").filter("creationdate <",
datetime.datetime.now() - datetime.timedelta(seconds=270))
Copy link
Member

Choose a reason for hiding this comment

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

Hi @ArneGudermann, can you comment this thing in the source-code? A reference to this issue would also be well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request Priority: Low This issue can be considered with enough idle time.
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants