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.

Sign up for GitHub

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

Jump to bottom

[integration] Add messaging etcd integration tests #15010

Closed
wants to merge 10 commits into from
Closed

[integration] Add messaging etcd integration tests #15010

wants to merge 10 commits into from

Conversation

llIlll
Copy link
Contributor

@llIlll llIlll commented Apr 3, 2022

Fixes #14295

Motivation

Adding Ectd integration tests #14295

Modifications

Adding Ectd integration tests

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Adding Ectd integration tests

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

  • 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 (if you have committer privilege).

Need to update docs?

  • doc-required

  • no-need-doc

  • doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 3, 2022
@llIlll
Copy link
Contributor Author

llIlll commented Apr 3, 2022

There is a remaining problem, the bug of bookkeeper 4.14.4 will cause the ci to fail, and it needs to be solved

issue: apache/bookkeeper#3011

HQebupt
HQebupt reviewed Apr 3, 2022
View reviewed changes
managed-ledger/pom.xml Outdated
@@ -99,14 +99,14 @@
</exclusions>
</dependency>
<dependency>
<groupId>io.dropwizard.metrics</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to keep it as it is because it has nothing to do with this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
@@ -143,11 +143,7 @@ public ProxyServiceStarter(String[] args) throws Exception {
// load config file
config = PulsarConfigurationLoader.create(configFile, ProxyConfiguration.class);

if (isBlank(metadataStoreUrl)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use zookeeperServers from command line if metadataStoreUrl is empty. How to handle if the metadataStoreUrl is empty ?

Copy link
Contributor Author

@llIlll llIlll Apr 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed.

RobertIndie
RobertIndie reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

There is a remaining problem, the bug of bookkeeper 4.14.4 will cause the ci to fail, and it needs to be solved

Does the problem here refer to the problem that caused the current CI failed?

  org.apache.pulsar.client.api.PulsarClientException$BrokerPersistenceException: {"errorMsg":"org.apache.bookkeeper.mledger.ManagedLedgerException: Not enough non-faulty bookies available","reqId":2486145185344564130, "remote":"localhost/127.0.0.1:49173", "local":"/127.0.0.1:43276"}

I saw that there are several zk integration tests that also failed. It looks like apache/bookkeeper#3011 should only affect cases that don't use zk as the metadata store. Seems there is something wrong with this PR. We should at least not let original tests fail.

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyServiceStarter.java
} else {
// Use metadataStoreUrl from command line
if (!isBlank(zookeeperServers)) {
config.setZookeeperServers(zookeeperServers);
Copy link
Member

Choose a reason for hiding this comment

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

We have deprecated the zookeeperServers in the proxy configuration in #13777. We would better not set it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously AbstractZkLedgerManager was used to create LedgerManagerFactory. In this issue, MetadataDriver was used to create LedgerManagerFactory, but their uri were different, so the zk test case would fail.

.../integration/src/test/java/org/apache/pulsar/tests/integration/topologies/PulsarCluster.java
this.zkContainer = new ZKContainer(clusterName);
this.metadataStoreContainer = createMetadataStoreContainer();
Copy link
Member

Choose a reason for hiding this comment

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

Is that we still need to maintain the ZK container even though we use the etcd container?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some test cases will use zk, such as TestBaseOffload.

.../integration/src/test/java/org/apache/pulsar/tests/integration/topologies/PulsarCluster.java
this.zkContainer
.withNetwork(network)
.withNetworkAliases(appendClusterName(ZKContainer.NAME))
.withEnv("clusterName", clusterName)
.withEnv("zkServers", appendClusterName(ZKContainer.NAME))
.withEnv("configurationStore", CSContainer.NAME + ":" + CS_PORT)
.withEnv("forceSync", "no")
.withEnv("pulsarNode", appendClusterName("pulsar-broker-0"));
.withEnv("pulsarNode", appendClusterName("pulsar-broker-0"))
Copy link
Member

Choose a reason for hiding this comment

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

Can we wrap the zkConainer here as a metadataStoreContainer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some test cases will use zk, such as TestBaseOffload.

.../integration/src/test/java/org/apache/pulsar/tests/integration/topologies/PulsarCluster.java
@@ -133,10 +139,11 @@ private PulsarCluster(PulsarClusterSpec spec, CSContainer csContainer, boolean s
this.proxyContainer = new ProxyContainer(appendClusterName("pulsar-proxy"), ProxyContainer.NAME)
.withNetwork(network)
.withNetworkAliases(appendClusterName("pulsar-proxy"))
.withEnv("configurationStoreServers", CSContainer.NAME + ":" + CS_PORT)
.withEnv("clusterName", clusterName)
.withEnv("zkServers", appendClusterName(ZKContainer.NAME))
.withEnv("zookeeperServers", appendClusterName(ZKContainer.NAME))
Copy link
Member

Choose a reason for hiding this comment

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

Since we deprecated the zookeeperServers, I think we can remove it here. The same is true for the broker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for not removing it is that watch-znode.py will check the zk status and can remove it if it has no other effect.

@RobertIndie RobertIndie added area/test type/feature The PR added a new feature or issue requested a new feature area/metadata labels Apr 6, 2022
@llIlll llIlll closed this May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HQebupt HQebupt HQebupt left review comments

@RobertIndie RobertIndie RobertIndie left review comments

Assignees

@llIlll llIlll

Labels
area/metadata area/test doc-not-needed Your PR changes do not impact docs type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding Ectd integration tests
3 participants
@llIlll @HQebupt @RobertIndie