You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/02/10 16:46:24 UTC
[GitHub] [pulsar] nodece opened a new pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
nodece opened a new pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225
### Motivation
The `schema-compatibility-strategy` cmd should be in the `topicsPolicies` subcommand, it is topic polcies.
### Modifications
- Move the schema compatibility strategy cmd from topics to topicsPolicies
- Fix the schema compatibility strategy test
### Documentation
Need to update docs?
- [x] `doc-required`
Need to update the `schema compatibility strategy` cmd.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r805128880
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,53 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
import org.awaitility.Awaitility;
import org.testng.annotations.Test;
public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
- @Override
- public void setupCluster() throws Exception {
- super.setupCluster();
- }
-
- @Override
- public void tearDownCluster() throws Exception {
- super.tearDownCluster();
- }
-
@Test
- public void testSchemaCompatibilityCmd() throws Exception {
- String topicName = generateTopicName("",true);
+ public void testSchemaCompatibilityStrategyCmd() throws Exception {
+ String topicName = generateTopicName("test-schema-compatibility-strategy", true);
pulsarAdmin.topics().createNonPartitionedTopic(topicName);
- Awaitility.await().untilAsserted(()->{
Review comment:
Only topic policies are asynchronous in Pulsar, so I remove this`Awaitility`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r804359982
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -245,10 +245,6 @@ public CmdTopics(Supplier<PulsarAdmin> admin) {
jcommander.addCommand("set-replication-clusters", new SetReplicationClusters());
jcommander.addCommand("remove-replication-clusters", new RemoveReplicationClusters());
- jcommander.addCommand("remove-schema-compatibility-strategy", new RemoveSchemaCompatibilityStrategy());
Review comment:
@codelipenghui Could you confirm this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806403190
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
There are some tests that will affect the results of another test, like I set schema compatibility strategy at namespace level in `testSchemaCompatibilityStrategyCmdWithNamespaceLevel`, if this test failed, it will break the `testSchemaCompatibilityStrategyCmd` test result, so I use `@BeforeMethod(alwaysRun = true)` instead of `@BeforeClass(alwaysRun = true)` to avoid this thing, each test have own Pulsar is safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1035930098
Hi @nodece since this PR contains doc changes on https://pulsar.apache.org/tools/pulsar-admin/2.10.0-SNAPSHOT/#topicpolicies, please label this kind of PR w/ `doc` in the future, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1039248812
@michaeljmarshall Please help review this PR again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806063357
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Is this method supposed to be annotated with `@BeforeClass`? `super.before()` is annotated that way. Given that `enableTopicPolicies()` sets broker config and brokers are only set up once per class, I think we want `@BeforeClass`.
```suggestion
@BeforeClass(alwaysRun = true)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r804822301
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,53 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
import org.awaitility.Awaitility;
import org.testng.annotations.Test;
public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
- @Override
- public void setupCluster() throws Exception {
- super.setupCluster();
- }
-
- @Override
- public void tearDownCluster() throws Exception {
- super.tearDownCluster();
- }
-
@Test
- public void testSchemaCompatibilityCmd() throws Exception {
- String topicName = generateTopicName("",true);
+ public void testSchemaCompatibilityStrategyCmd() throws Exception {
+ String topicName = generateTopicName("test-schema-compatibility-strategy", true);
pulsarAdmin.topics().createNonPartitionedTopic(topicName);
- Awaitility.await().untilAsserted(()->{
Review comment:
Why remove the `Awaitility` usage here? I assume this was present because there is a propagation race for the strategy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1035963034
@Anonymitaet Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1035909419
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] momo-jun commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
momo-jun commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1035834890
> Hi @momo-jun can you follow up on the doc change? Thanks
Sure, I will take care of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r804820484
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -245,10 +245,6 @@ public CmdTopics(Supplier<PulsarAdmin> admin) {
jcommander.addCommand("set-replication-clusters", new SetReplicationClusters());
jcommander.addCommand("remove-replication-clusters", new RemoveReplicationClusters());
- jcommander.addCommand("remove-schema-compatibility-strategy", new RemoveSchemaCompatibilityStrategy());
Review comment:
Thank you for confirming. I didn't realize that when I left my review. We need to make sure this gets merged before 2.10 is released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1038709361
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806536332
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Your worry is necessary. I know that reset broker is somewhat expensive. The `PulsarCliTestSuite` is abstract for cli test, normal we don't override the `before` and `after` methods, which need to be overridden when tests need isolation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1039248812
@michaeljmarshall Please help review this PR again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806063357
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Is this method supposed to be annotated with `@BeforeClass`? `super.before()` is annotated that way. Given that `enableTopicPolicies()` sets broker config and brokers are only set up once per class, I think we want `@BeforeClass`.
```suggestion
@BeforeClass(alwaysRun = true)
```
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Okay, I can see how state of individual tests can impact each other. However, the current design of the `PulsarCliTestSuite` does not align with its usage here. Is there a way to ensure test isolation without resetting the cluster? If not, I think it'd be better to make it clearer that the `PulsarCliTestSuite#before` method should be called before each test and that the abstract suite is designed to make individual tests isolated.
A secondary concern is that it has to be somewhat expensive to reset brokers. We should try to limit the cost of basic tests, if possible.
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Sure, let's create an issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r804318315
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -245,10 +245,6 @@ public CmdTopics(Supplier<PulsarAdmin> admin) {
jcommander.addCommand("set-replication-clusters", new SetReplicationClusters());
jcommander.addCommand("remove-replication-clusters", new RemoveReplicationClusters());
- jcommander.addCommand("remove-schema-compatibility-strategy", new RemoveSchemaCompatibilityStrategy());
Review comment:
Have these commands released? If so, please add these commands to `initDeprecatedCommands`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1038843605
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806403190
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
There are some tests that will affect the results of another test, like I set schema compatibility strategy at namespace level in `testSchemaCompatibilityStrategyCmdWithNamespaceLevel`, if this test failed, it will break the `testSchemaCompatibilityStrategyCmd` test result, so I use `@BeforeMethod(alwaysRun = true)` instead of `@BeforeClass(alwaysRun = true)` to avoid this thing, each test have own Pulsar is safe.
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Your worry is necessary. I know that reset broker is somewhat expensive. The `PulsarCliTestSuite` is abstract for cli test, normal we don't override the `before` and `after` methods, which need to be overridden when tests need isolation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r804385145
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
##########
@@ -245,10 +245,6 @@ public CmdTopics(Supplier<PulsarAdmin> admin) {
jcommander.addCommand("set-replication-clusters", new SetReplicationClusters());
jcommander.addCommand("remove-replication-clusters", new RemoveReplicationClusters());
- jcommander.addCommand("remove-schema-compatibility-strategy", new RemoveSchemaCompatibilityStrategy());
Review comment:
These commands were added by https://github.com/apache/pulsar/pull/13297 . And hasn't been released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1038634722
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806878163
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
@michaeljmarshall Can we unblock the 2.10.0 release first and create a new issue to track the test improvement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicsPolicies
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1035810230
Hi @momo-jun can you follow up on the doc change? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1038765370
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r807039545
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Sure, let's create an issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806463723
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
Okay, I can see how state of individual tests can impact each other. However, the current design of the `PulsarCliTestSuite` does not align with its usage here. Is there a way to ensure test isolation without resetting the cluster? If not, I think it'd be better to make it clearer that the `PulsarCliTestSuite#before` method should be called before each test and that the abstract suite is designed to make individual tests isolated.
A secondary concern is that it has to be somewhat expensive to reset brokers. We should try to limit the cost of basic tests, if possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#discussion_r806878163
##########
File path: tests/integration/src/test/java/org/apache/pulsar/tests/integration/cli/topicpolicies/SchemaCompatibilityStrategyTest.java
##########
@@ -19,67 +19,68 @@
package org.apache.pulsar.tests.integration.cli.topicpolicies;
-import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.assertEquals;
import org.apache.pulsar.common.policies.data.SchemaCompatibilityStrategy;
import org.apache.pulsar.tests.integration.docker.ContainerExecResult;
-import org.apache.pulsar.tests.integration.suites.PulsarTestSuite;
+import org.apache.pulsar.tests.integration.suites.PulsarCliTestSuite;
import org.awaitility.Awaitility;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-public class SchemaCompatibilityStrategyTest extends PulsarTestSuite {
+public class SchemaCompatibilityStrategyTest extends PulsarCliTestSuite {
+ @BeforeMethod(alwaysRun = true)
Review comment:
@michaeljmarshall Can we unblock the 2.10.0 release first and create a new issue to track the test improvement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #14225: [Broker] Move schema compatibility strategy cmd from topics to topicPolicies
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #14225:
URL: https://github.com/apache/pulsar/pull/14225#issuecomment-1041008022
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org