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/03/01 11:30:03 UTC
[GitHub] [pulsar] nicoloboschi opened a new pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
nicoloboschi opened a new pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508
### Motivation
This is the same fix applied here #12461. The problem with the other pull is that the `AbstractSchema#clone()` method does return the same instance, so the fix is not useful at all.
### Modifications
* Create a new INT schema for the test purpose
- [x] `no-need-doc`
--
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 #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816976159
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
Great point. In order to get this test passing, we can avoid the mutability issues for `Schema.INT32`.
--
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] nicoloboschi commented on pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1055682978
@michaeljmarshall @eolivelli I created this issue https://github.com/apache/pulsar/issues/14522 for continuing the discussion
--
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] eolivelli commented on a change in pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816950832
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
if you don't want to open the Pandora box of clone() then please use Schema.JSON() that is guaranteed to always return a new instance
--
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 merged pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
michaeljmarshall merged pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508
--
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] nicoloboschi commented on pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1056759244
/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] eolivelli commented on a change in pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r817131978
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -709,7 +708,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
+ final Schema<Integer> integerSchema = Schema.JSON(Integer.class);
Review comment:
Please update the comment above
--
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] lhotari commented on pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1055383429
> The problem with the other pull is that the AbstractSchema#clone() method does return the same instance, so the fix is not useful at all.
I wonder why https://github.com/apache/pulsar/blob/16beb9d97fdc64092c8f3fe6959d6bf20dd0aa13/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/AbstractSchema.java#L68-L70 doesn't implement clone? It seems really misleading to have such a method on the API if it's not doing what it's supposed to do. #6356 added the clone method. Perhaps @congbobo184 could explain the reasoning?
--
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 edited a comment on pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
michaeljmarshall edited a comment on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1055658603
Note also that the properties map (the original issue from #12461) is a mutable map that is likely passed among threads:
https://github.com/apache/pulsar/blob/89a36f9e4a0cd37676b7501184004f01a7c47150/pulsar-common/src/main/java/org/apache/pulsar/client/impl/schema/SchemaInfoImpl.java#L62-L66
Depending on how we access this map, that could be its own issue.
That `SchemaInfoImpl` class doesn't appear to be using safe publication of state either, which makes the mutability a larger problem.
--
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] nicoloboschi commented on pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1055442534
/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] nicoloboschi commented on a change in pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816974088
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
@michaeljmarshall good catch! you're right
I think the test issue can be just fixed as @eolivelli suggested, using a JSON schema. The type of the schema is not relevant for test itself.
I agree that the `clone()` method returns`this` because, probably, the initial intention was to keep it immutable.
Moving the getter to return an immutable map may be the right solution, even if it can be considered breaking change because can cause runtime error while upgrading the client
--
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] nicoloboschi commented on a change in pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816974088
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
@michaeljmarshall good catch! you're right
I think this issue can be just fixed as @eolivelli suggested, using a JSON schema. The type of the schema is not relevant for test itself.
I agree that the `clone()` method return `this` because probably the intention was to keep it immutable.
Moving the getter to return an immutable map may be the right solution, even if it can be considered breaking change because can cause runtime error while upgrading the client
--
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] congbobo184 commented on pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1057649360
i think primitive schema also can implement clone
--
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] congbobo184 commented on pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1057650924
> > The problem with the other pull is that the AbstractSchema#clone() method does return the same instance, so the fix is not useful at all.
>
> I wonder why
>
> https://github.com/apache/pulsar/blob/16beb9d97fdc64092c8f3fe6959d6bf20dd0aa13/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/AbstractSchema.java#L68-L70
>
> doesn't implement clone? It seems really misleading to have such a method on the API if it's not doing what it's supposed to do. #6356 added the clone method. Perhaps @congbobo184 could explain the reasoning?
There will be some components inside and will change. When the user wants to use the component at that time, we need the clone method to get the current schema, and it will not affect the original schema. Doesn't seem to be of much use in primitive schema as they are all the same
--
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] eolivelli commented on a change in pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816948939
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
This is not good.
it means that users that use the public API Schema.INT32 and use clone() see surprises.
I believe that the problem is that clone() is not working properly
Schema are stateful objects in Pulsar and clone() is very important
--
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 #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816960837
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
I don't believe this solution works. `SCHEMA_INFO` is static, so any modification of it in one test will affect other tests. This is a design flaw.
A larger question for me is why we're letting users modify the `SCHEMA_INFO` object. It is static, so cloning isn't going to do anything.
--
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] nicoloboschi commented on a change in pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816974088
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
@michaeljmarshall good catch! you're right
I think the test issue can be just fixed as @eolivelli suggested, using a JSON schema. The type of the schema is not relevant for test itself.
I agree that the `clone()` method returns`this` because, probably, the initial intention was to keep it immutable.
Moving the getter to return an immutable map may be the right solution, even if it can be considered a breaking change because can cause runtime error while upgrading the client
--
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] nicoloboschi commented on a change in pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#discussion_r816974088
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
##########
@@ -708,7 +709,7 @@ public void testNullKeyValueProperty() throws PulsarAdminException, PulsarClient
map.put(null, "value"); // null key is not allowed for JSON, it's only for test here
// leave INT32 instance unchanged
- final Schema<Integer> integerSchema = Schema.INT32.clone();
+ final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();
Review comment:
@michaeljmarshall good catch! you're right
I think the test issue can be just fixed as @eolivelli suggested, using a JSON schema. The type of the schema is not relevant for test itself.
I agree that the `clone()` method return `this` because probably the intention was to keep it immutable.
Moving the getter to return an immutable map may be the right solution, even if it can be considered breaking change because can cause runtime error while upgrading the client
--
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 pull request #14508: [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14508:
URL: https://github.com/apache/pulsar/pull/14508#issuecomment-1055658603
Note also that the properties map (the original issue from #12461) is a mutable map that is likely passed among threads:
https://github.com/apache/pulsar/blob/89a36f9e4a0cd37676b7501184004f01a7c47150/pulsar-common/src/main/java/org/apache/pulsar/client/impl/schema/SchemaInfoImpl.java#L62-L66
Depending on how we access this map, that could be its own 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