Skip to content

Conversation

@michaeljmarshall
Copy link
Member

Similar to: #20142

Motivation

In #20142 we changed the Consumer and the Producer logic to call the correct AuthorizationService method.

Our goal is to deprecate the AuthorizationService methods for canProduce and canConsume, so this change helps us move in the right direction.

This PR follows the same logic and updates the WebSocket proxy to remove all calls to the can* methods in the AuthorizationService

Modifications

  • Update ProducerHandler, ConsumerHandler, and ReaderHander in the WebSocket Proxy to call the AuthorizationService#allowTopicOperationAsync method.

Verifying this change

This change is trivial.

Documentation

  • doc-not-needed

Matching PR in forked repository

PR in forked repository: Skipping PR as I ran tests locally.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari closed this May 17, 2023
@lhotari lhotari reopened this May 17, 2023
@lhotari
Copy link
Member

lhotari commented May 17, 2023

@michaeljmarshall build must be retried in 3 days until completion. After that, it's necessary to rebuild all

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #20299 (f80cc11) into master (bc1764f) will increase coverage by 37.74%.
The diff coverage is 25.45%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20299       +/-   ##
=============================================
+ Coverage     34.48%   72.22%   +37.74%     
+ Complexity    12528     4327     -8201     
=============================================
  Files          1614     1869      +255     
  Lines        126177   141439    +15262     
  Branches      13772    15999     +2227     
=============================================
+ Hits          43508   102157    +58649     
+ Misses        77051    31094    -45957     
- Partials       5618     8188     +2570     
Flag Coverage Δ
inttests 24.18% <18.18%> (+0.04%) ⬆️
systests 25.09% <9.09%> (?)
unittests 71.42% <25.45%> (+38.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...a/org/apache/pulsar/websocket/ConsumerHandler.java 62.50% <0.00%> (+35.91%) ⬆️
...a/org/apache/pulsar/websocket/ProducerHandler.java 67.81% <0.00%> (+34.88%) ⬆️
...ava/org/apache/pulsar/websocket/ReaderHandler.java 47.05% <0.00%> (+47.05%) ⬆️
...r/broker/authentication/AuthenticationService.java 69.52% <37.50%> (+35.52%) ⬆️
...oker/service/nonpersistent/NonPersistentTopic.java 73.75% <66.66%> (+23.33%) ⬆️
...rvice/nonpersistent/NonPersistentSubscription.java 53.29% <100.00%> (+16.04%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 74.42% <100.00%> (+14.22%) ⬆️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 77.89% <100.00%> (+30.61%) ⬆️

... and 1503 files with indirect coverage changes

@michaeljmarshall
Copy link
Member Author

Thanks @lhotari. I forgot to follow up on this PR.

@michaeljmarshall
Copy link
Member Author

/pulsarbot rerun-failure-checks

1 similar comment
@michaeljmarshall
Copy link
Member Author

/pulsarbot rerun-failure-checks

@michaeljmarshall
Copy link
Member Author

michaeljmarshall commented May 18, 2023

The pulsar io phase failed on the upload step:

   ###### End Diagnostic HTTP information ######
  /home/runner/work/_temp/_github_home/.local/bin/gh-actions-artifact-client.js:649
              throw Error(`${name} failed: ${customErrorInformation}`);
                    ^
  
  Error: Create Artifact Container failed: The artifact name coverage_and_deps_unittest_PULSAR_IO.tar.zst is not valid. Request URL https://pipelines.actions.githubusercontent.com/EXIZfX8ixSbQEfqDA00dMJrEtloTMSwjlUs9NqJqrTcJfd7NaR/_apis/pipelines/workflows/5007643515/artifacts?api-version=6.0-preview
      at /home/runner/work/_temp/_github_home/.local/bin/gh-actions-artifact-client.js:649:19
      at Generator.next (<anonymous>)
      at fulfilled (/home/runner/work/_temp/_github_home/.local/bin/gh-actions-artifact-client.js:597:58)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

@michaeljmarshall
Copy link
Member Author

/pulsarbot rerun-failure-checks

@michaeljmarshall michaeljmarshall merged commit e4f1f09 into apache:master May 18, 2023
@michaeljmarshall michaeljmarshall deleted the ws-authz-handers branch May 18, 2023 05:27
michaeljmarshall added a commit that referenced this pull request May 18, 2023
Relates to: #20299

### Motivation

We have several catch blocks that are dedicated to providing meaningful logs about timeouts. As such, we should catch the `TimeoutException`, not the `InterruptedException`, in order to ensure accurate logs.

### Modifications

* Replace `InterruptedException` with `TimeoutException`

### Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

### Documentation

- [x] `doc-not-needed`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/websocket doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants