-
Notifications
You must be signed in to change notification settings - Fork 4
Keep context for last 50 executions and actionId #4
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
Conversation
frikky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(:
shuffle_sdk/shuffle_sdk.py
Outdated
| self.start_time = int(time.time_ns()) | ||
| fullKey = str(self.current_execution_id) + "-" + str(self.action["id"]) | ||
|
|
||
| if fullKey in pastAppExecutions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a log here. It's VERY important to know when this happens as to track it at scale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup Yup
shuffle_sdk/shuffle_sdk.py
Outdated
| return | ||
|
|
||
| pastAppExecutions.append(fullKey) | ||
| pastAppExecutions = pastAppExecutions[-50:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't update every time. Due to the parallel way things run in Flask/Werkzeug it should happen rarely as to not fuck up the threading/concurrency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does -50 work when the array is empty? Seems like an out of bounds to me, idk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shuffle_sdk/shuffle_sdk.py
Outdated
|
|
||
| # Make start time with milliseconds | ||
| self.start_time = int(time.time_ns()) | ||
| fullKey = str(self.current_execution_id) + "-" + str(self.action["id"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you 100% sure "id" ALWAYS exists in self.action? In every scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! yes you're right let me add some checks and try-expect
|
Tell me when this is ready :)) |
|
@frikky ready for review :) |
frikky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the print change, then I'll merge :)
shuffle_sdk/shuffle_sdk.py
Outdated
| action_id = self.action.get("id") | ||
| fullKey = str(self.current_execution_id) + "-" + str(action_id) | ||
| if fullKey in pastAppExecutions: | ||
| self.logger.info(f"[INFO] Duplicate execution detected for execution id - {self.current_execution_id} and action - {self.action["id"]}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we make this an [ERROR] as it should NEVER EVER occur. That way we can see it easily.
|
Done @frikky 👍 |
frikky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay edgecases
shuffle_sdk/shuffle_sdk.py
Outdated
| fullKey = str(self.current_execution_id) + "-" + str(action_id) | ||
| if fullKey in pastAppExecutions: | ||
| self.logger.info(f"[INFO] Duplicate execution detected for execution id - {self.current_execution_id} and action - {self.action["id"]}") | ||
| self.logger.error(f"[ERROR] Duplicate execution detected for execution id - {self.current_execution_id} and action - {self.action["id"]}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing:
- Since you're using
action_idabove, why do you use self.action["id"] below? self.action.get("id") is safe, but self.action["id"] is not
This may lead to it continuing due to action ID failing to load in the 2nd one 😆
|
Fixed :) @frikky |

No description provided.