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.

Sign up for GitHub

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

Jump to bottom

[Servicenow] Fix end signal not enqueued #2546

Merged
merged 6 commits into from
May 25, 2024
Merged

[Servicenow] Fix end signal not enqueued #2546

merged 6 commits into from
May 25, 2024

Conversation

praveen-elastic
Copy link
Collaborator

@praveen-elastic praveen-elastic commented May 13, 2024 •
edited

Closes #2545

Following changes are included in this PR:

  1. Ensure end-signals are put to queue even if error is encountered, in all relevant places.
  2. Set advanced_rules_enabled to True

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • Considered corresponding documentation changes
  • Contributed any configuration settings changes to the configuration reference
  • if you added or changed Rich Configurable Fields for a Native Connector, you made a corresponding PR in Kibana

Changes Requiring Extra Attention

  • Security-related changes (encryption, TLS, SSRF, etc)
  • New external service dependencies added.

Related Pull Requests

Release Note

@praveen-elastic praveen-elastic marked this pull request as ready for review May 13, 2024 08:42
@praveen-elastic praveen-elastic requested a review from a team as a code owner May 13, 2024 08:42
seanstory
seanstory reviewed May 13, 2024
View reviewed changes
Copy link
Member

@seanstory seanstory left a 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?

connectors/sources/servicenow.py
)
self.task_count += 1
except Exception as exception:
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

@praveen-elastic praveen-elastic enabled auto-merge (squash) May 25, 2024 03:05
@praveen-elastic praveen-elastic merged commit 2be17b5 into main May 25, 2024
2 checks passed
@praveen-elastic praveen-elastic deleted the praveen/servicenow-fix-endsignal-enqueued branch May 25, 2024 03:23
github-actions bot pushed a commit that referenced this pull request May 25, 2024
@praveen-elastic
[Servicenow] Fix end signal not enqueued (#2546)
6cd8937
@github-actions github-actions bot mentioned this pull request May 25, 2024
Merged
github-actions bot pushed a commit that referenced this pull request May 25, 2024
@praveen-elastic
[Servicenow] Fix end signal not enqueued (#2546)
dca32ee
@github-actions github-actions bot mentioned this pull request May 25, 2024
Merged
@github-actions GitHub Actions
Copy link

💚 Backport PR(s) successfully created

Status Branch Result
✅ 8.14 #2579
✅ 8.13 #2580

The backport PRs will be merged automatically after passing CI.

artem-shelkovnikov pushed a commit that referenced this pull request May 30, 2024
@github-actions @praveen-elastic
[8.13] [Servicenow] Fix end signal not enqueued (#2546) (#2580)
d65981b 
Co-authored-by: Praveen Kukreja <90465691+praveen-elastic@users.noreply.github.com>
artem-shelkovnikov pushed a commit that referenced this pull request May 30, 2024
@github-actions @praveen-elastic
[8.14] [Servicenow] Fix end signal not enqueued (#2546) (#2579)
4157fbb 
Co-authored-by: Praveen Kukreja <90465691+praveen-elastic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanstory seanstory seanstory approved these changes

Assignees
No one assigned
Labels
auto-backport v8.13.4 v8.14.0 v8.15.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Servicenow] Syncronization job never finishes if getting the attachment list fails
2 participants
@praveen-elastic @seanstory