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.

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

[fix][client] Fix conversion of TimestampMillisConversion has no effect when Jsr310Conversion enabled #15863

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented May 31, 2022

Motivation

Fixes #15858

The conversion that is registered first is a higher priority than the registered later, so TimestampMillisConversion should not be registered after TimestampMicrosConversion.

Modifications

Improve avro conversion order of registration.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@coderzc coderzc changed the title Fix avro conversion order of registration [fix][client] Fix avro conversion order of registration May 31, 2022
@coderzc coderzc changed the title [fix][client] Fix avro conversion order of registration [fix][client] improve avro conversion order of registration May 31, 2022
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

good work!
can you change the title explaining the issue ?
improve avro conversion order of registration seems an improvement and it doesn't talk about the timestamp problem

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 31, 2022
@coderzc coderzc changed the title [fix][client] improve avro conversion order of registration [fix][client] Fix conversion of TimestampMillisConversion has no effect when Jsr310Conversion enabled May 31, 2022
@coderzc coderzc changed the title [fix][client] Fix conversion of TimestampMillisConversion has no effect when Jsr310Conversion enabled [fix][client] Fix conversion of TimestampMillisConversion has no effect when Jsr310Conversion enabled May 31, 2022
@coderzc
Copy link
Member Author

coderzc commented May 31, 2022

good work! can you change the title explaining the issue ? improve avro conversion order of registration seems an improvement and it doesn't talk about the timestamp problem

ok, I changed the title

@codelipenghui codelipenghui added this to the 2.11.0 milestone Jun 1, 2022
@codelipenghui codelipenghui merged commit 311fdb5 into apache:master Jun 1, 2022
@fmiguelez
Copy link
Contributor

It would be great to see this fix also applied to at least 2.9.x and 2.8.x branches. Is 2.7.x no longer supported?

hangc0276 pushed a commit that referenced this pull request Jun 7, 2022
### Motivation

Fixes #15858

The conversion that is registered first is a higher priority than the registered later, so `TimestampMillisConversion` should not be registered after `TimestampMicrosConversion`.

### Modifications

Improve `avro` conversion order of registration.

(cherry picked from commit 311fdb5)
@hangc0276 hangc0276 added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jun 7, 2022
codelipenghui pushed a commit to codelipenghui/incubator-pulsar that referenced this pull request Jun 7, 2022
### Motivation

Fixes apache#15858

The conversion that is registered first is a higher priority than the registered later, so `TimestampMillisConversion` should not be registered after `TimestampMicrosConversion`.

### Modifications

Improve `avro` conversion order of registration.

(cherry picked from commit 311fdb5)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2022
### Motivation

Fixes apache#15858

The conversion that is registered first is a higher priority than the registered later, so `TimestampMillisConversion` should not be registered after `TimestampMicrosConversion`.

### Modifications

Improve `avro` conversion order of registration.

(cherry picked from commit 311fdb5)
(cherry picked from commit 7377a59)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2022
### Motivation

Fixes apache#15858

The conversion that is registered first is a higher priority than the registered later, so `TimestampMillisConversion` should not be registered after `TimestampMicrosConversion`.

### Modifications

Improve `avro` conversion order of registration.

(cherry picked from commit 311fdb5)
(cherry picked from commit 784c9c8)
codelipenghui pushed a commit that referenced this pull request Jun 10, 2022
### Motivation

Fixes #15858

The conversion that is registered first is a higher priority than the registered later, so `TimestampMillisConversion` should not be registered after `TimestampMicrosConversion`.

### Modifications

Improve `avro` conversion order of registration.

(cherry picked from commit 311fdb5)
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schema cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.8.4 release/2.9.3 release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
6 participants