Skip to content

NIFI-12092 Add backoff parameters to JettyWebsocketClient reconnect#7761

Closed
Lehel44 wants to merge 4 commits intoapache:mainfrom
Lehel44:NIFI-12092
Closed

NIFI-12092 Add backoff parameters to JettyWebsocketClient reconnect#7761
Lehel44 wants to merge 4 commits intoapache:mainfrom
Lehel44:NIFI-12092

Conversation

@Lehel44
Copy link
Contributor

@Lehel44 Lehel44 commented Sep 19, 2023

Summary

NIFI-12092

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@ChrisSamo632 ChrisSamo632 changed the title Add backoff parameters to JettyWebsocketClient reconnect NIFI-12092 Add backoff parameters to JettyWebsocketClient reconnect Sep 20, 2023
connectCount = configurationContext.getProperty(CONNECTION_ATTEMPT_COUNT).evaluateAttributeExpressions().asInteger();
maximumBackoffMillis = configurationContext.getProperty(MAXIMUM_BACKOFF_TIME).asTimePeriod(TimeUnit.MILLISECONDS);
initialBackoffMillis = configurationContext.getProperty(INITIAL_BACKOFF_TIME).asTimePeriod(TimeUnit.MILLISECONDS);
backoffJitterMillis = (long) (initialBackoffMillis * BACKOFF_JITTER_FACTOR * ThreadLocalRandom.current().nextDouble(-1, 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally we want to avoid using ThreadLocal as a mechanism in NiFi though in this case it might be fine. That said what is the need for a random number generated here? Can we just toss out this random and have an explicit value?

Copy link
Contributor Author

@Lehel44 Lehel44 Sep 22, 2023

Choose a reason for hiding this comment

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

Since it is only used at the initialization I can change it to Random to adhere to the guidelines.

There's a maintaining thread (maintainSessions method) that periodically checks and re-establishes connections for active sessions. When there are numerous active sessions, this could strain the server. By introducing randomness or jitter to the retry intervals, clients are less likely to retry simultaneously, spreading out the load more evenly and reducing the chances of congestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest I would keep it simple.
I wouldn't make this more complicated for the users unless really necessary.

The number of connection attempts are limited by the number of timer driven threads and most likely the real number of concurrent attempts would be much lower.
We really only need a delay and a simple hard-coded 1-2 sec should be suffice in my opinion. We can add a very simple jitter via a simple Random if we really want.

@asfgit asfgit closed this in 7e4ca93 Oct 4, 2023
@tpalfy
Copy link
Contributor

tpalfy commented Oct 4, 2023

LGTM
Thank you for your work @Lehel44 and @joewitt for the review.
Merged into main.

asfgit pushed a commit that referenced this pull request Oct 5, 2023
This closes #7761.

Signed-off-by: Tamas Palfy <tpalfy@apache.org>

(cherry picked from commit 7e4ca93)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants