NIFI-9448 Improve S2S HTTP Extend Transaction Exception Handling#5577
Closed
exceptionfactory wants to merge 1 commit intoapache:mainfrom
Closed
NIFI-9448 Improve S2S HTTP Extend Transaction Exception Handling#5577exceptionfactory wants to merge 1 commit intoapache:mainfrom
exceptionfactory wants to merge 1 commit intoapache:mainfrom
Conversation
- Refactor background transaction extension to ExtendTransactionCommand - Avoid closing S2S HTTP client for IllegalStateExceptions - Avoid creating additional S2S HTTP client instance for transaction extension commands - Add check for extend transaction requests received in client test class - Add null check for Peer Persistence implementation in PeerSelector
gresockj
approved these changes
Dec 8, 2021
Contributor
gresockj
left a comment
There was a problem hiding this comment.
Thanks for this improvement, @exceptionfactory. I think this makes the transaction extension code more intuitive, and removes the additional client created for transaction extension. I was able to reproduce the issue on a cluster under heavy S2S load by using a remote debugger, closing the HttpClient right before extending the transaction, and did not detect any adverse behavior.
Will merge.
asfgit
pushed a commit
that referenced
this pull request
Dec 14, 2021
- Refactor background transaction extension to ExtendTransactionCommand - Avoid closing S2S HTTP client for IllegalStateExceptions - Avoid creating additional S2S HTTP client instance for transaction extension commands - Add check for extend transaction requests received in client test class - Add null check for Peer Persistence implementation in PeerSelector Signed-off-by: Joe Gresock <jgresock@gmail.com> This closes #5577.
krisztina-zsihovszki
pushed a commit
to krisztina-zsihovszki/nifi
that referenced
this pull request
Jun 28, 2022
- Refactor background transaction extension to ExtendTransactionCommand - Avoid closing S2S HTTP client for IllegalStateExceptions - Avoid creating additional S2S HTTP client instance for transaction extension commands - Add check for extend transaction requests received in client test class - Add null check for Peer Persistence implementation in PeerSelector Signed-off-by: Joe Gresock <jgresock@gmail.com> This closes apache#5577.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
NIFI-9448 Improves Site-to-Site HTTP client processing for extend transaction exception handling.
As part of sending or receive files over an HTTP S2S connection, the
SiteToSiteRestApiClientstarts a background command to request extension of the current transaction on a periodic basis. In situations when the underlying HTTP connection pool throws anIllegalStateException, the background command attempts to close the parentSiteToSiteRestApiClient. This behavior short-circuits potential retry attempts that might otherwise be allowed according to the configured timeout settings.These changes include refactoring the background command to a separate
ExtendTransactionCommandclass for easier testing. With the mainSiteToSiteRestApiClientalready wrapping an Apache HttpComponentsHttpClientinstance and connection pool, the refactoredExtendTransactionCommandreuses the same client and connection pool as opposed to creating a new instance ofSiteToSiteRestApiClient. Additional changes include a null check inPeerSelectorto avoid a potentialNullPointerExceptionwhen the Peer Persistence property is not configured. This pull request also removes theSiteToSiteClientITas it does not provide significant value beyond the existing unit tests.In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically
main)?Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not
squashor use--forcewhen pushing to allow for clean monitoring of changes.For code changes:
mvn -Pcontrib-check clean installat the rootnififolder?LICENSEfile, including the mainLICENSEfile undernifi-assembly?NOTICEfile, including the mainNOTICEfile found undernifi-assembly?.displayNamein addition to .name (programmatic access) for each of the new properties?For documentation related changes:
Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.