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

fix(polling): respect request timeout settings #8391

Merged
2 commits merged into from
Dec 20, 2021
Merged

fix(polling): respect request timeout settings #8391

2 commits merged into from
Dec 20, 2021

Conversation

romansmirnov
Copy link
Member

@romansmirnov romansmirnov commented Dec 14, 2021 •
edited
Loading

Description

  • If long polling is disabled by the received request, then always complete the request immediately even when no jobs are activated.
  • Ensure that the provided request timeout is respected so that the request completes at latest at the given timeout.

Related issues

relates #8310
closes #8389

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement

@romansmirnov
Copy link
Member Author

As an alternative approach, I was thinking of initializing the request's timer (see #addTimeOut()) immediately when receiving the request at the beginning (in #activateJobs(ActivateJobsRequest, ServerStreamObserver<ActivateJobsResponse>)). So that, it is not necessary anymore to check for it when resubmitting the request over and over again (as done within this PR).

Given the assumption that typically there is always work to do, i.e., there are "always" available jobs to activate, the requests will succeed in activating jobs most of the time with its first execution. Only, in "exceptional" cases the request won't succeed, meaning, there is not enough (or nothing) to do so that the request must be repeated by the gateway later.

So, to avoid the overhead of adding a timer to the request, I have decided to add the timer only when the request could not activate jobs with its first execution. As a consequence, only in the "exceptional" cases, the timer is added (and only when it wasn't added previously). Of course, the request would time out at maximum requestTimeout + firstRequestExecutionTime, but IMO that's okay and can be ignored for now.

@romansmirnov romansmirnov force-pushed the 8389-lp-respect-request-timeout branch 2 times, most recently from 1579217 to 8bde1c9 Compare December 16, 2021 13:20
@romansmirnov romansmirnov marked this pull request as ready for review December 16, 2021 14:42
lenaschoenburg
lenaschoenburg reviewed Dec 20, 2021
View reviewed changes
Copy link
Member

@lenaschoenburg lenaschoenburg left a comment

Choose a reason for hiding this comment

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

LGTM Roman, thanks for fixing. 🏅 I'm approving this now and backporting to 1.1, 1.2 and for the 1.3 release branch.

TBH it took me a good while to understand the interaction between the state and handler and also the control flow between activateJobs and completeOrResubmitRequest. Maybe this is something we could improve in a future PR.

bors r+

@ghost
Copy link

ghost commented Dec 20, 2021

👎 Rejected by too few approved reviews

@lenaschoenburg
Copy link
Member

bors r+

@ghost
Copy link

ghost commented Dec 20, 2021

Build succeeded:

@ghost ghost merged commit 2c754a7 into develop Dec 20, 2021
@ghost ghost deleted the 8389-lp-respect-request-timeout branch December 20, 2021 14:57
@github-actions github-actions bot mentioned this pull request Dec 20, 2021
Merged
@github-actions
Copy link
Contributor

Successfully created backport PR #8449 for stable/1.1.

@github-actions github-actions bot mentioned this pull request Dec 20, 2021
Merged
@github-actions
Copy link
Contributor

Successfully created backport PR #8450 for stable/1.2.

@github-actions github-actions bot mentioned this pull request Dec 20, 2021
Merged
@github-actions
Copy link
Contributor

Successfully created backport PR #8451 for release-1.3.0.

ghost pushed a commit that referenced this pull request Dec 20, 2021
@dependabot @menski
@romansmirnov @lenaschoenburg
merge: #8448 #8450
724d3df 
8448: [Backport stable/1.2] deps(maven): bump version.elasticsearch from 7.16.1 to 7.16.2 r=menski a=npepinpe

Bumps `version.elasticsearch` from 7.16.1 to 7.16.2.

Updates `elasticsearch-x-content` from 7.16.1 to 7.16.2
- [Release notes](https://github.com/elastic/elasticsearch/releases)
- [Commits](elastic/elasticsearch@v7.16.1...v7.16.2)

Updates `elasticsearch-rest-client` from 7.16.1 to 7.16.2
- [Release notes](https://github.com/elastic/elasticsearch/releases)
- [Commits](elastic/elasticsearch@v7.16.1...v7.16.2)

---
updated-dependencies:
- dependency-name: org.elasticsearch:elasticsearch-x-content
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: org.elasticsearch.client:elasticsearch-rest-client
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
(cherry picked from commit 4f41e4f)

8450: [Backport stable/1.2] fix(polling): respect request timeout settings r=oleschoenburg a=github-actions[bot]

# Description
Backport of #8391 to `stable/1.2`.

relates to #8310 #8389

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sebastian Menski <sebastian.menski@camunda.com>
Co-authored-by: Roman <roman.smirnov@camunda.com>
Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
ghost pushed a commit that referenced this pull request Dec 20, 2021
@dependabot @menski
@romansmirnov @lenaschoenburg
merge: #8447 #8449
f9a9f1f 
8447: [Backport stable/1.1] deps(maven): bump version.elasticsearch from 7.16.1 to 7.16.2 r=menski a=npepinpe

Bumps `version.elasticsearch` from 7.16.1 to 7.16.2.

Updates `elasticsearch-x-content` from 7.16.1 to 7.16.2
- [Release notes](https://github.com/elastic/elasticsearch/releases)
- [Commits](elastic/elasticsearch@v7.16.1...v7.16.2)

Updates `elasticsearch-rest-client` from 7.16.1 to 7.16.2
- [Release notes](https://github.com/elastic/elasticsearch/releases)
- [Commits](elastic/elasticsearch@v7.16.1...v7.16.2)

---
updated-dependencies:
- dependency-name: org.elasticsearch:elasticsearch-x-content
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: org.elasticsearch.client:elasticsearch-rest-client
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
(cherry picked from commit 4f41e4f)

8449: [Backport stable/1.1] fix(polling): respect request timeout settings r=oleschoenburg a=github-actions[bot]

# Description
Backport of #8391 to `stable/1.1`.

relates to #8310 #8389

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sebastian Menski <sebastian.menski@camunda.com>
Co-authored-by: Roman <roman.smirnov@camunda.com>
Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
ghost pushed a commit that referenced this pull request Dec 20, 2021
@romansmirnov @lenaschoenburg
merge: #8451
81fc843 
8451: [Backport release-1.3.0] fix(polling): respect request timeout settings r=oleschoenburg a=github-actions[bot]

# Description
Backport of #8391 to `release-1.3.0`.

relates to #8310 #8389

Co-authored-by: Roman <roman.smirnov@camunda.com>
Co-authored-by: Ole Schönburg <ole.schoenburg@gmail.com>
@korthout korthout added the version:1.3.0 Marks an issue as being completely or in parts released in 1.3.0 label Jan 4, 2022
@Zelldon Zelldon mentioned this pull request Jan 4, 2022
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lenaschoenburg lenaschoenburg lenaschoenburg approved these changes

Assignees
No one assigned
Labels
version:1.3.0 Marks an issue as being completely or in parts released in 1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Gateway does not respect the long polling timeout received along with the activate jobs request
4 participants
@romansmirnov @lenaschoenburg @menski @korthout