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.

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

[improve][pulsar-testclient] Add proxyServiceUrl and proxyProtocol as options for PerfTool CLI #4

Open
wants to merge 59 commits into
base: master
Choose a base branch
from

Conversation

vineeth1995
Copy link
Owner

Motivation

The pulsar-perf-tool should be able to take "proxyServiceUrl" and "proxyProtocol" as an input arguments as it can be provided by the user.

Modifications

Add "proxyServiceUrl" and "proxyProtocol" into PerformanceBaseArguments.java class.
Make changes into PerfClientUtils.java to pass "proxyServiceUrl" and "proxyProtocol" to clientBuilder.
Add unit test cases to verify

Verifying this change

This change added tests and can be verified as follows:

-Added unit test to verify functionality

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): ( no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

}

if (proxyProtocol == null) {
proxyProtocol = ProxyProtocol.valueOf(prop.getProperty("proxyProtocol"));
Copy link
Owner Author

Choose a reason for hiding this comment

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

should I handle exception here if user provides some different option in config?

coderzc and others added 29 commits September 27, 2022 06:50
)

* Fix maxNumberOfRejectedRequestPerConnection doc

* fix doc in 2.8.x docs
…ed (apache#17704)

* [fix][metrics]wrong metrics text generated when label_cluster specified

* improve logic branch

* mark test group
… been published after the topic gets activated on a broker (apache#16618)

* Skip creating a replication snapshot if no messages have been published

* Adapt test to new behavior where replication snapshots happen only when there are new messages
* Call cleanup method in finally block to ensure it's not skipped

* Clear invocations for the mocks that are left around without cleanup

* Cleanup PulsarService and PulsarAdmin mocks/spies in MockedPulsarServiceBaseTest

* Don't record invocations at all for PulsarService and PulsarAdmin in MockedPulsarServiceBaseTest

* Don't record invocations for spies by default

* Simplify reseting mocks

* Fix PersistentTopicTest

* Fix TokenExpirationProducerConsumerTest

* Fix SimpleLoadManagerImplTest

* Fix FilterEntryTest
…he#17834)

- use Bookkeeper defaults by setting BK_METADATA_OPTIONS=none
…7209)

Fixes apache#17186

### Motivation

There are some cases in which it is useful to be able to include current
position of the message when reset of cursor was made.

### Modifications

* Support inclusive seek in c++ consumers.
* Add a unit test to verify.
* fix: delete sqlite files after jdbc connection closed

This closes apache#17713.

Signed-off-by: tison <[email protected]>

* uses isolated db file

Signed-off-by: tison <[email protected]>

* Revert "uses isolated db file"

This reverts commit 295db3c.

* close in order

Signed-off-by: tison <[email protected]>

* strong order guarantee

Signed-off-by: tison <[email protected]>

* factor out defer logic to avoid further bugs

Signed-off-by: tison <[email protected]>

* Revert "factor out defer logic to avoid further bugs"

This reverts commit f7f4634.

* Revert "strong order guarantee"

This reverts commit 747086f.

* use awaitTermination

Signed-off-by: tison <[email protected]>

Signed-off-by: tison <[email protected]>
…AndCommitForTransaction (apache#17845)

* scenario is already covered by PendingAckPersistentTest
…ManagedLedgerImpl (apache#17293)

- a NPE with no description is confusing
…on time (apache#17790)

Fixes
- apache#17623
- apache#17637

### Motivation

Manually release resources, including `consumer`, `producer`, `pulsar client`, `transaction`, and `topic`. This saves `setup` and `cleanup` time before and after each method. 

### Modifications

- Manually release resources instead of calling `cleanup` & `setup` each method
- remove useless method `markDeletePositionCheck`
- `Integer.valueOf(int)` instead of `new Integer(int)`, because `new Integer(int)` is deprecated

### Matching PR in forked repository

PR in forked repository: 

- poorbarcode#10
Fixes apache#17785

### Motivation

The `failureMap` need to be clear after run per unit test.

### Modifications

Clear `failureMap` after run per unit test, and only run once `setup()`/`cleanup()` to reduce execution time.

### Matching PR in forked repository

PR in forked repository: coderzc#6
…ache#17252)

- fixes issue with stats where timestamps might be inconsistent because of visibility issues
  - fields should be volatile to ensure visibility of updated values in a consistent manner

- in replication, the lastDataMessagePublishedTimestamp field in PersistentTopic might be inconsistent
  unless volatile is used
…he#16891)

* add reader config doc

* update to the versioned doc

* Update site2/docs/io-debezium-source.md

Co-authored-by: momo-jun <[email protected]>

* Update site2/docs/io-debezium-source.md

Co-authored-by: momo-jun <[email protected]>

* revert changes to 2.10.1 and 2.9.3

Co-authored-by: momo-jun <[email protected]>
Signed-off-by: Zixuan Liu <[email protected]>

Signed-off-by: Zixuan Liu <[email protected]>

### Motivation

Improve the compactor tool, using separate TLS config

### Modifications

- Add separate TLS config on the compactor, both Keystore and PEM formats are supported
- Fix correct use of service URL by `brokerConfig.isBrokerClientTlsEnabled()` value

### Verifying this change

Test has been added.
 
### Documentation

Check the box below or label this PR directly.

Need to update docs? 

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)
  
- [x] `doc-not-needed` 
(Please explain why)
  
- [ ] `doc` 
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)
…ts (apache#17686)

* [feat][broker] Add config to count filtered entries towards rate limits

* Make fixes for checkstyle

* Remove * import

* Fix incorrect conflict resolution in merge commit

### Motivation

Currently, when using entry filters, filtered out messages do not count against the rate limit. Therefore, a subscription that is completely filtered will never be throttled due to rate limiting. When the messages are delivered to the consumer for a filtered subscription, those messages will count against the rate limit, and in that case, the message filtering can be throttled because the check to delay `readMoreEntries()` happens before message filtering. Therefore, the rate limit will essentially be increased as a function of the percent of messages let through the filter (some quick math is that the new rate is likely `dispatchRate * (1 / percentDelivered)`, where percent delivered is a percent as a decimal).

It's possible that some use cases prefer this behavior, but in my case, I think it'd be valuable to include these filtered messages in the dispatch throttling because these messages still cost the broker network, memory, and cpu. This PR adds a configuration to count filtered out messages towards dispatch rate limits for the broker, the topic, and the subscription.

### Modifications

* Add configuration named `dispatchThrottlingForFilteredEntriesEnabled`. Default it to false so we maintain the original behavior. When true, count filtered messages against rate limits.
* Refactor the code to `acquirePermitsForDeliveredMessages` so that it is in the `AbstractBaseDispatcher`, which makes it available to the entry filtering logic.

### Verifying this change

A new test is added as part of this PR.

### Does this pull request potentially affect one of the following parts:

This PR introduces a new config while maintaining the current behavior.

### Documentation

- [x] `doc-not-needed` 
Config docs are auto-generated.
…ache#17837)

Fixes: apache#14109


### Motivation

The expected execution flow for this test is: 

1. send 505 messages
2. commit 10 transactions, every transaction ack 50 messages
3. receive the last 5 messages in the last transaction, wait for transaction timeout
4. confirm that the last 5 messages can be consumed by new consumer

<strong>(High light)</strong> The default value for transaction TTL is 10 seconds, and the default value for `Awaitility.await` is also 10 seconds,  so this test is not stable.

Note: This is a guess cause, the problem is not reproduced locally. But after transaction TTL is set to 11s, the probability of the problem occurring is 100%.

### Modifications

Fix flaky test
- set transaction TTL to 5s

Other changes
- define a name for the task thread
- acknowledge the last 5 messages

### Matching PR in forked repository

PR in forked repository: 

- poorbarcode#13
…trics (apache#17701)

Master Issue: apache#15370

### Modifications

- Make transaction `MLTransactionMetadataStoreProvider` & `MLPendingAckStoreProvider` support buffered writer metrics.
  - Motivation: apache#15370

----

- Delete constructor of `TxnLogBufferedWriter` without parameter `metrics`.
  - Motivation: it is unnecessary.

---- 

- Add a default `DisabledTxnLogBufferedWriterMetricsStats` implementation.

----

- Previous PR remaining code to optimize: remove the check code `if (metrics != null)`. The motivation see:
  - Motivation: apache#16758 (comment)

----

- Make transaction log buffered writer only create by the `MLTransactionMetadataStoreProvider` & `MLPendingAckStoreProvider`. 
  - Motivation: apache#16758 (comment)

### Documentation

- [ ] `doc-required` 

- [x] `doc-not-needed` 

- [ ] `doc` 

- [ ] `doc-complete`


### Matching PR in forked repository

PR in forked repository: 

- poorbarcode#3
congbobo184 and others added 24 commits September 29, 2022 23:53
Fixes: apache#15773  apache#16863 apache#16860

### Motivation
```
  Error:  Tests run: 11, Failures: 1, Errors: 0, Skipped: 3, Time elapsed: 87.06 s <<< FAILURE! - in org.apache.pulsar.packages.management.storage.bookkeeper.BookKeeperPackagesStorageTest
  Error:  setUp(org.apache.pulsar.packages.management.storage.bookkeeper.BookKeeperPackagesStorageTest)  Time elapsed: 13.089 s  <<< FAILURE!
  org.apache.zookeeper.KeeperException$ConnectionLossException: KeeperErrorCode = ConnectionLoss
  	at org.apache.zookeeper.KeeperException.create(KeeperException.java:102)
  	at org.apache.bookkeeper.zookeeper.ZooKeeperWatcherBase.waitForConnection(ZooKeeperWatcherBase.java:159)
  	at org.apache.bookkeeper.zookeeper.ZooKeeperClient$Builder.build(ZooKeeperClient.java:260)
  	at org.apache.bookkeeper.test.ZooKeeperUtil.restartCluster(ZooKeeperUtil.java:133)
  	at org.apache.bookkeeper.test.ZooKeeperUtil.startCluster(ZooKeeperUtil.java:104)
  	at org.apache.pulsar.packages.management.storage.bookkeeper.bookkeeper.test.BookKeeperClusterTestCase.startZKCluster(BookKeeperClusterTestCase.java:238)
  	at org.apache.pulsar.packages.management.storage.bookkeeper.bookkeeper.test.BookKeeperClusterTestCase.setUp(BookKeeperClusterTestCase.java:178)
  	at org.apache.pulsar.packages.management.storage.bookkeeper.bookkeeper.test.BookKeeperClusterTestCase.setUp(BookKeeperClusterTestCase.java:166)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
  	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
  	at org.testng.internal.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:61)
  	at org.testng.internal.ConfigInvoker.invokeConfigurationMethod(ConfigInvoker.java:366)
  	at org.testng.internal.ConfigInvoker.invokeConfigurations(ConfigInvoker.java:320)
  	at org.testng.internal.TestInvoker.runConfigMethods(TestInvoker.java:701)
  	at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:527)
  	at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
  	at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
  	at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
  	at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
  	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
  	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
  	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
  	at org.testng.TestRunner.privateRun(TestRunner.java:764)
  	at org.testng.TestRunner.run(TestRunner.java:585)
  	at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
  	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
  	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
  	at org.testng.SuiteRunner.run(SuiteRunner.java:286)
  	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
  	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
  	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
  	at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
  	at org.testng.TestNG.runSuites(TestNG.java:1069)
  	at org.testng.TestNG.run(TestNG.java:1037)
  	at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:112)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeLazy(TestNGDirectoryTestSuite.java:123)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:90)
  	at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
  	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
  	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
  	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
  	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
```

The root cause is that the zk client randomly selects IPV4 and IPV6 when parsing localhost, can connect when using IPV4, and fails when using IPV6. Therefore, if you continue to randomly connect to IPV6, the connection will timeout.

https://github.com/apache/zookeeper/blob/bc1b231c9e32667b2978c86a6a64833470973dbd/zookeeper-server/src/main/java/org/apache/zookeeper/client/StaticHostProvider.java#L140-L146
Thanks to @poorbarcode  for helping me locate the problem

### Modifications
add     @AfterMethod(alwaysRun = true)
use Adress replace hostName

### Documentation

- [x] `doc-not-needed` 

### Matching PR in the forked repository

PR in forked repository: 

- congbobo184#1
* PIP-209: Removed C++/Python clients from main repo

* Removed python directory from Docekrfile

* Fixed python client version argument scoping

* Fixed handling of pulsar.functions.serde
…apache#17893)

Fixes: apache#16864

### Motivation

I think it is a wrong configuration(`ackTimeout 1s`) when writing the code, the original design is set `negativeAckRedeliveryDelay 1s`

The process expects:

- send 10 messages in one batch
  - submit a batch. 
- receive 10 messages, do negative acknowledge
- after `1s`, will trigger `redelivery`
- receive 10 messages again

The real process:
- send 1 message
  - Reach the batch time limit, and submit a batch. return `msgId_1`
- send 9 messages in another batch
  - submit a batch. return `msgId_2`
- receive 10 messages, do negative acknowledge
  - push the `msgId_1` to `negativeAcksTracker`
  - push the `msgId_2` to `unAckedMessageTracker`
- after `1s`, will trigger redelivery `msgId_2` by `unAckedMessageTracker`
- receive 9 messages( `msgId_2` ) again
- after `60s`, will trigger redelivery `msgId_1` by `negativeAcksTracker`. <strong>(High light)</strong> Test execution timeout!
- receive 1 messages( `msgId_1` ) again



### Modifications

- remove conf: `ackTimeout`
- set `negativeAckRedeliveryDelay 1s`


### Documentation

- [x] `doc-not-needed` 

### Matching PR in forked repository

PR in forked repository: 

- poorbarcode#18
* improve TLS encryption

* fix review comments

* preview fix

* add file and syntax for page redirection

* update the process to create PEM certs

* add more description for mTLS
…he#17855)

* [fix] Remove pulsar-broker-common dependency from pulsar-client

* fix newline

* add enforcer rule

* Move packages-core to jdk8 bytecode

* checkstyle

* use variables

* style

* Fix annotation discovery

* Fix kafka module compile
@github-actions
Copy link

github-actions bot commented Nov 3, 2022

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.