Fix/issue/19268 #20
Open
Fix/issue/19268 #20
Conversation
This file contains 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
…ctive broker. (apache#19244) Co-authored-by: nicklixinyang <nicklixinyang@didiglobal.com>
… parallel forked test procs (apache#19252)
…eightedMaxEMA) to BrokerLoadData (apache#19154)
…o deprecate `zookeeperAllowReadOnlyOperations` (apache#19246)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
…he first file rotation (apache#19247)
…pache#19197) PIP: apache#12105 ### Motivation This is the first of several PRs to implement [PIP 97](apache#12105). This PR seeks to start to solve the fact that the `AuthenticationState` class currently authenticates `authData` twice instead of just once. This change is important to make before we are able to utilize the async methods introduced in apache#12104. Historical context: apache#14044 introduced the `AuthenticationProvider#newHttpAuthState` method. The only use case for this method in the pulsar code base is to let custom providers specify the `AuthenticationDataSource` on http request attributes. The primary problem with that implementation is that the `AuthenticationState` class currently involves authenticating the `authData` passed in to the `newHttpAuthState`. As such, this code is sub-optimal, and creates a confusing flow. I propose we deprecate the `newHttpAuthState` method and instead start using the `authenticateHttpRequestAsync` and `authenticateHttpRequest` methods to allow custom implementations to configure the desired `AuthenticationDataSource` on the request attributes. In order to simplify the diff for reviewers, this PR uses the deprecated `AuthenticationProvider#authenticateHttpRequest` method. I plan to follow up and switch to use the `AuthenticationProvider#authenticateHttpRequestAsync` method. Note that these changes are completely backwards compatible. The only risk is to users that have custom code loaded into the broker that calls the `AuthenticationProvider#authenticateHttpRequest` method. ### Modifications * Deprecate `AuthenticationService#authenticateHttpRequest(HttpServletRequest request, AuthenticationDataSource authData)`. It is no longer used. * Deprecate `AuthenticationProvider#newHttpAuthState(HttpServletRequest request)`. It is no longer used outside of the `AuthenticationProvider` interface itself. * Remove `@InterfaceStability.Unstable` annotation from the `authenticateHttpRequestAsync` method. When I introduced that annotation, I was under the impression that we didn't need it. However, in order to meet the requirements introduced in apache#14044, we need to let custom `AuthenticationProviders` add their own attributes. * Update the default implementation of `authenticateHttpRequest`. Because the previous implementation was unreachable by all auth providers except for the SASL implementation, this is a backwards compatible change. * Implement changes for the `AuthenticationProviderToken` so that it no longer relies on `newHttpAuthState`. ### Verifying this change I added new tests. ### Does this pull request potentially affect one of the following parts: - [x] The public API This changes the public API within the broker by marking some methods as `@Deprecated`. ### Documentation - [x] `doc-not-needed` We document the `AuthenticationProvider` interface in the code. I added these docs. There is currently no where else to update docs. ### Matching PR in forked repository PR in forked repository: michaeljmarshall#12 ### Additional motivation from PR discussion My primary motivation for this PR is to implement PIP 97. If we want authentication to be asynchronous, we cannot assume that when the `AuthenticationState` object is initialized, the `authenticationDataSource` and the `authRole` are present because the authentication might not yet be completed. My goal with this PR is to break an unnecessary relationship between `AuthenticationState` and http request authentication that was created by apache#14044.
…che#19278) ### Motivation The `AuthenticationDataSource` variable was added to the `ProxyConnection` class in apache#1200. It is no longer needed. Note that we store this variable in the `ServerCnx` because it is used for authorization. Because we do not do authorization in the proxy, we don't need it. ### Modifications * Remove unused variable ### Verifying this change This is a trivial change. ### Does this pull request potentially affect one of the following parts: The only conceivable way this is a breaking change is if a third party implementation implemented their library so that `authState.getAuthDataSource()` has a side effect. ### Documentation - [x] `doc-not-needed` <!-- Your PR changes do not impact docs --> No docs needed.
…on instead of IllegalStateException (apache#19284)
…apache#19275) ### Motivation #### 1. `readPosition` point to a deleted ledger When `trim ledgers` and `create new cursor` are executed concurrently, it will cause the `readPosition` of the cursor to point to a deleted ledger. | time | `trim ledgers` | `create new cursor` | | --- | --- | --- | | 1 | | set read position and mark deleted position | | 2 | delete ledger | | | 3 | | add the cursor to `ManagedLedger.cursors` | ---- #### 2. Backlog wrong caused by `readPosition` wrong <strong>(Highlight)</strong>Since the read position of the cursor is pointing at a deleted ledger, so deleted messages will never be consumed or acknowledged. Since the backlog in the API `topics stats` response is calculated as this: `managedLedger.entriesAddedCounter - cursor.messagesConsumedCounter`, the result is: Topics stats show `msgBacklog` but there is reality no backlog. - `managedLedger.entriesAddedCounter`: Pulsar will set it to `0` when creating a new managed ledger, it will increment when adding entries. - `cursor.messagesConsumedCounter`: Pulsar will set it to `0` when creating a new cursor, it will increment when acknowledging. For example: - write entries to the managed ledger: `{1:0~1:9}...{5:0~5:9}` - `managedLedger.entriesAddedCounter` is `50` now - create a new cursor, and set the read position to `1:0` - `cursor.messagesConsumedCounter` is `0` now - delete ledgers `1~4` - consume all messages - can only consume the messages {5:0~5:9}, so `cursor.messagesConsumedCounter` is `10` now - the `backlog` in response of `topics stats` is `50 - 10 = 40`, but there reality no backlog ---- #### 3. Reproduce issue Sorry, I spent 4 hours trying to write a non-invasive test, but failed. <strong>(Highlight)</strong>You can reproduce by `testBacklogIfCursorCreateConcurrentWithTrimLedger` in the PR apache#19274 https://github.com/apache/pulsar/blob/a2cdc759fc2710e4dd913eb0485d23ebcaa076a4/pulsar-broker/src/test/java/org/apache/bookkeeper/mledger/impl/StatsBackLogTest.java#L163 ### Modifications Avoid the race condition of `cursor.initializeCursorPosition` and `internalTrimLedgers`
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
…ermediate build artifacts (apache#19305)
…he#19312) PIP: apache#12105 ### Motivation When authentication fails in the `ServerCnx`, the state is left in `Start` if the primary `authData` fails authentication and in `Connecting` or `Connected` if the `originalAuthData` authentication fails. To prevent any kind of unexpected behavior, we should go to `Failed` state. Note that the tests verify the current behavior where a failed `originalAuthData` results first in a `Connected` command from the broker and then an `Error` command. I documented that I think this is sub optimal here apache#19311. ### Modifications * Update `ServerCnx` state to `Failed` when there is an authentication exception during `handleConnect` and during `handleAuthResponse`. * Update `handleAuthResponse` reply to `"Unable to authenticate"` instead of the `AuthenticationState` exception. ### Verifying this change A new test is added. The added test covers the change made in apache#19295 where we updated `ServerCnx` so that we call `AuthState#authenticate` instead of relying on the implementation detail that the initialization calls `authenticate`. That PR should have added a test. ### Does this pull request potentially affect one of the following parts: This is not a breaking change. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: michaeljmarshall#18
… getters in tests (apache#19323)
… prevent StackOverflowError in compilation (apache#19324)
…iness in all metric / stat tests (apache#19329)
…ersistent policies cannot set to < 0 (apache#18999)
…s incorrect (apache#19301) Co-authored-by: zhangzhanchang <zhangzhanchang@didiglobal.com>
…backlog-size of admin API topics stats true (apache#19302)
…dCursor(single subscription check) upon subscription (apache#19343)
|
@labuladong Please add the following content to your PR description and select a checkbox: |
15 tasks
|
The pr had no activity for 30 days, mark with Stale label. |
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.
Fixes #xyz
Master Issue: #xyz
PIP: #xyz
Motivation
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: