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

Update pulsar dependency to 2.10.3 #144

Merged
merged 6 commits into from
Apr 14, 2023
Merged

Update pulsar dependency to 2.10.3 #144

merged 6 commits into from
Apr 14, 2023

Conversation

aymkhalil
Copy link
Contributor

@aymkhalil aymkhalil commented Apr 11, 2023

2.8 has performance reported issue, recommended to update to 2.10.x. Please note that the connector pulsar client will be vended by the environment, but this upgrade benefits the agent. It also unified the client version between the Agent and CLI tool.

Notes:

  • The maxPendingMessagesAcrossPartitions has been deprecated as of pulsar 2.10 (see PIP-120)
  • I couldn't upgrade to 2.11 because it required java 17. CDC need to compile with java 11 too. Sample build error:
bad class file: /home/runner/.gradle/caches/modules-2/files-2.1/org.apache.pulsar/pulsar-functions-api/2.11.0/6abd8cd6cf9b9a6ef15a7e53882577a897dcaf8a/pulsar-functions-api-2.11.0.jar(/org/apache/pulsar/functions/api/KVRecord.class)
    class file has wrong version 61.0, should be 55.0

@aymkhalil aymkhalil force-pushed the pulsar-2.11 branch 2 times, most recently from 1fd2e90 to a0fcd0b Compare April 11, 2023 23:43
@aymkhalil aymkhalil changed the title Update pulsar dependency to 2.11 Update pulsar dependency to 2.10.3 Apr 11, 2023
@aymkhalil aymkhalil marked this pull request as ready for review April 12, 2023 00:05
Copy link
Collaborator

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Switching to 2.10.x requires more work.

The default settings changed a lot, by default now there is a limit on the memory used by the producer and not on the number of pending messages.

I suggest to apply a default configuration similar to 2.8.3

Please take a look to the changes we did in CNDB in order to achieve better results.

The gist of it is to disable the memory limit and configure the max pending messages

@aymkhalil
Copy link
Contributor Author

@eolivelli

  • The maxPendingMessage is configured on the producer and has default value already
  • CNDB uses client 2.10.0 that doesn't have the fix for the memory limit. This patch uses 2.10.3 which has the fix
  • I added the configuration for memoryLimit that is disabled by default anyway.
  • maxPendingMessagesAcrossPartitions is deprecated and I think it is better to drop it as I don't expect future support for it

PTAL.

@aymkhalil aymkhalil requested a review from eolivelli April 12, 2023 17:47
@aymkhalil
Copy link
Contributor Author

aymkhalil commented Apr 12, 2023

Behavior of the JsonNode node returned in the NativeObject has changes. I have to tests failing now with:

java.lang.AssertionError: Wrong value for regular field double expected:<1.0> but was:<1>

With 2.8.3, I would get "1.0" when accessing node.numberValue() with DOUBEL schema. Now I get an "1" integer.

Need to troubleshoot further...

Update: Regression was introduced in 2.10.1. I captured the issue here apache/pulsar#20092

In the mean time, I downgraded the client in the tests to 2.8.3 (like it was before). This Patch is still meaning full because it successfully update the client on the agent side to 2.10.3 and the connector client is only used in the integration tests.

Copy link
Collaborator

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

@eolivelli eolivelli merged commit 3db4951 into master Apr 14, 2023
@eolivelli eolivelli deleted the pulsar-2.11 branch April 14, 2023 06:22
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.

2 participants