-
Notifications
You must be signed in to change notification settings - Fork 115
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
[Servicenow] Fix end signal not enqueued #2546
[Servicenow] Fix end signal not enqueued #2546
Conversation
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.
Can you add unit tests too?
) | ||
self.task_count += 1 | ||
except Exception as exception: |
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.
This feels like it should still fail the sync, no?
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.
Hey, I didn't understand what you are trying to convey.
These changes are similar to the ones we implemented in confluence #2380.
Using these changes we make sure that END_SIGNALS for a given task are added to queue even if error occurs inside that task, this prevents _consumer method from running in a forever while loop.
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.
In that linked PR, the caught exceptions are logged with _logger.exception
and then re-raise
'd. Here, you're just swallowing the error, so the sync won't fail after this is merged, but the same issue would fail the sync today.
Unless you've got a good reason (which you might!), I suggest re-raising the error, and logging these as error/exception instead of warn.
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.
Got it, i'll update the code to re-raise the error
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.
@seanstory updated the PR with the changes.
Closes #2545
Following changes are included in this PR:
Checklists
Pre-Review Checklist
config.yml.example
)v7.13.2
,v7.14.0
,v8.0.0
)Changes Requiring Extra Attention
Related Pull Requests
Release Note