[flaky-tests] AdminApiSchemaTest#testSchemaInfoApi by nicoloboschi · Pull Request #14508 · apache/pulsar

nicoloboschi · 2022-03-01T11:29:49Z

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.

I see this test also failing in 2.8 and 2.9 branch, I recommend to cherry-pick it.

Modifications

Create a new INT schema for the test purpose

no-need-doc

lhotari · 2022-03-01T12:16:03Z

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

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/AbstractSchema.java

Lines 68 to 70 in 16beb9d

    
           public Schema<T> clone() { 
        
               return this; 
        
           }

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?

nicoloboschi · 2022-03-01T13:27:12Z

/pulsarbot rerun-failure-checks

eolivelli · 2022-03-01T16:42:02Z


        // leave INT32 instance unchanged
-        final Schema<Integer> integerSchema = Schema.INT32.clone();
+        final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();


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

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

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.

@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

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

michaeljmarshall

@lhotari - it's likely that clone returns this because it is supposed to be immutable. Unfortunately, the SCHEMA_INFO is static and mutable, so this bug would exist even if clone worked properly.

@nicoloboschi - great work identifying this issue. I think we'll need to discuss a large solution. In my mind, the SCHEMA_INFO shouldn't be modifiable in this case.

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/IntSchema.java

Lines 31 to 40 in 16beb9d

    
           private static final IntSchema INSTANCE; 
        
           private static final SchemaInfo SCHEMA_INFO; 
        
           static { 
        
               SCHEMA_INFO = new SchemaInfoImpl() 
        
                   .setName("INT32") 
        
                   .setType(SchemaType.INT32) 
        
                   .setSchema(new byte[0]); 
        
               INSTANCE = new IntSchema(); 
        
           }

michaeljmarshall · 2022-03-01T16:54:41Z


        // leave INT32 instance unchanged
-        final Schema<Integer> integerSchema = Schema.INT32.clone();
+        final Schema<Integer> integerSchema = DefaultImplementation.getDefaultImplementation().newIntSchema();


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.

michaeljmarshall · 2022-03-01T17:04:16Z

Note also that the properties map (the original issue from #12461) is a mutable map that is likely passed among threads:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/client/impl/schema/SchemaInfoImpl.java

Lines 62 to 66 in 89a36f9

    
               /** 
        
                * Additional properties of the schema definition (implementation defined). 
        
                */ 
        
               @Builder.Default 
        
               private Map<String, String> properties = Collections.emptyMap();

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.

nicoloboschi · 2022-03-01T17:30:28Z

@michaeljmarshall @eolivelli I created this issue #14522 for continuing the discussion

nicoloboschi · 2022-03-02T10:23:18Z

/pulsarbot rerun-failure-checks

congbobo184 · 2022-03-03T04:17:21Z

i think primitive schema also can implement clone

congbobo184 · 2022-03-03T04:20:39Z

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

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/AbstractSchema.java

Lines 68 to 70 in 16beb9d

public Schema<T> clone() {

return this;

}

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

michaeljmarshall

LGTM

* [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi * use custom json schema * remove old comment ### 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. I see this test also failing in 2.8 and 2.9 branch, I recommend to cherry-pick it. ### Modifications * Create a new INT schema for the test purpose - [x] `no-need-doc` (cherry picked from commit 32c3cd1)

* [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi * use custom json schema * remove old comment ### Motivation This is the same fix applied here apache#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. I see this test also failing in 2.8 and 2.9 branch, I recommend to cherry-pick it. ### Modifications * Create a new INT schema for the test purpose - [x] `no-need-doc` (cherry picked from commit 32c3cd1) (cherry picked from commit 0f72a4f)

* [flaky-tests] AdminApiSchemaTest#testSchemaInfoApi * use custom json schema * remove old comment ### Motivation This is the same fix applied here apache#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. I see this test also failing in 2.8 and 2.9 branch, I recommend to cherry-pick it. ### Modifications * Create a new INT schema for the test purpose - [x] `no-need-doc`

[flaky-tests] AdminApiSchemaTest#testSchemaInfoApi

e89081d

github-actions Bot assigned nicoloboschi Mar 1, 2022

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Mar 1, 2022

lhotari approved these changes Mar 1, 2022

View reviewed changes

lhotari requested review from congbobo184, eolivelli and michaeljmarshall March 1, 2022 12:18

Jason918 approved these changes Mar 1, 2022

View reviewed changes

codelipenghui added this to the 2.11.0 milestone Mar 1, 2022

codelipenghui added release/2.10.1 release/2.8.4 release/2.9.3 labels Mar 1, 2022

codelipenghui approved these changes Mar 1, 2022

View reviewed changes

RobertIndie approved these changes Mar 1, 2022

View reviewed changes

eolivelli requested changes Mar 1, 2022

View reviewed changes

michaeljmarshall requested changes Mar 1, 2022

View reviewed changes

nicoloboschi mentioned this pull request Mar 1, 2022

SchemaInfoImpl instances are treated as immutable even if they are mutable #14522

Open

use custom json schema

37fa21b

sijie mentioned this pull request Mar 1, 2022

ISSUE-14522: SchemaInfoImpl instances are treated as immutable even if they are mutable streamnative/pulsar-archived#3841

Open

eolivelli approved these changes Mar 1, 2022

View reviewed changes

Comment thread pulsar-broker/src/test/java/org/apache/pulsar/schema/SchemaTest.java

remove old comment

00d9c65

nicoloboschi requested a review from michaeljmarshall March 2, 2022 07:43

congbobo184 approved these changes Mar 3, 2022

View reviewed changes

michaeljmarshall approved these changes Mar 7, 2022

View reviewed changes

michaeljmarshall merged commit 32c3cd1 into apache:master Mar 7, 2022

michaeljmarshall added cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.9.4 and removed release/2.9.3 labels Mar 7, 2022

michaeljmarshall added the cherry-picked/branch-2.10 label Mar 7, 2022

michaeljmarshall added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Mar 7, 2022

codelipenghui modified the milestones: 2.11.0, 2.10.0 Mar 12, 2022

codelipenghui added release/2.9.3 and removed cherry-picked/branch-2.10 release/2.9.4 release/2.10.1 labels Mar 12, 2022

	private static final IntSchema INSTANCE;
	private static final SchemaInfo SCHEMA_INFO;

	static {
	SCHEMA_INFO = new SchemaInfoImpl()
	.setName("INT32")
	.setType(SchemaType.INT32)
	.setSchema(new byte[0]);
	INSTANCE = new IntSchema();
	}

Uh oh!

Conversation

nicoloboschi commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Uh oh!

lhotari commented Mar 1, 2022

Uh oh!

nicoloboschi commented Mar 1, 2022

Uh oh!

eolivelli Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

eolivelli Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

nicoloboschi Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoloboschi commented Mar 1, 2022

Uh oh!

Uh oh!

nicoloboschi commented Mar 2, 2022

Uh oh!

congbobo184 commented Mar 3, 2022

Uh oh!

congbobo184 commented Mar 3, 2022

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nicoloboschi commented Mar 1, 2022 •

edited

Loading

nicoloboschi Mar 1, 2022 •

edited

Loading

michaeljmarshall left a comment •

edited

Loading

michaeljmarshall commented Mar 1, 2022 •

edited

Loading