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

[flaky-tests] AdminApiSchemaTest#testSchemaInfoApi #14508

Merged
merged 3 commits into from
Mar 7, 2022
Merged

[flaky-tests] AdminApiSchemaTest#testSchemaInfoApi #14508

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e89081d
[flaky-tests] AdminApiSchemaTest#testSchemaInfoApi
nicoloboschi Mar 1, 2022
37fa21b
use custom json schema
nicoloboschi Mar 1, 2022
00d9c65
remove old comment
nicoloboschi Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
import org.apache.pulsar.client.impl.schema.SchemaInfoImpl;
import org.apache.pulsar.client.impl.schema.generic.GenericJsonRecord;
import org.apache.pulsar.client.impl.schema.writer.AvroWriter;
import org.apache.pulsar.client.internal.DefaultImplementation;
import org.apache.pulsar.common.naming.TopicDomain;
import org.apache.pulsar.common.naming.TopicName;
import org.apache.pulsar.common.policies.data.ClusterData;
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nicoloboschi nicoloboschi Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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 returnsthis 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

Copy link
Member

Choose a reason for hiding this comment

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

Great point. In order to get this test passing, we can avoid the mutability issues for Schema.INT32.

((SchemaInfoImpl) integerSchema.getSchemaInfo()).setProperties(map);

final Consumer<Integer> consumer = pulsarClient.newConsumer(integerSchema).topic(topic)
Expand Down