Skip to content

[FLINK-9803] Drop canEqual() from TypeSerializer#7732

Closed
aljoscha wants to merge 1 commit intoapache:masterfrom
aljoscha:drop-can-equal-serializer
Closed

[FLINK-9803] Drop canEqual() from TypeSerializer#7732
aljoscha wants to merge 1 commit intoapache:masterfrom
aljoscha:drop-can-equal-serializer

Conversation

@aljoscha
Copy link
Contributor

@aljoscha aljoscha commented Feb 18, 2019

What is the purpose of the change

This change removes TypeSerializer.canEqual() because it is not useful on that class hierachy. Serializers can only equals() on an exact match, not with different serializers up and down the hierarchy.

Verifying this change

This is covered by existing tests. We simply remove the method in the interface and all subclasses.

@flinkbot
Copy link
Collaborator

flinkbot commented Feb 18, 2019

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

  • ❌ 1. The [description] looks good.
  • ❌ 2. There is [consensus] that the contribution should go into to Flink.
  • ❗ 3. Needs [attention] from.
  • ❌ 4. The change fits into the overall [architecture].
  • ❌ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot approve description to approve the 1st aspect (similarly, it also supports the consensus, architecture and quality keywords)
  • @flinkbot approve all to approve all aspects
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval

@aljoscha aljoscha force-pushed the drop-can-equal-serializer branch from 6090ff8 to a3b0f7c Compare February 18, 2019 09:57
@aljoscha
Copy link
Contributor Author

@flinkbot attention @tzulitai @igalshilman

@igalshilman
Copy link
Contributor

All and all, 👍 for removing dead code.

There might be a slight potential issue with InstantSerializer since it does not define a serialVersionUID and removing the canEqual will change the auto generated value.

Can you please add to InstantSeiralizer:
private static final long serialVersionUID = -4131715684999061277L;

Guess we'll have to wait for Travis, but other than that LGTM.

@aljoscha aljoscha force-pushed the drop-can-equal-serializer branch from 49e984d to efc169f Compare February 19, 2019 10:00
This change removes TypeSerializer.canEqual() because it is not useful
on that class hierachy. Serializers can only equals() on an exact
match, not with different serializers up and down the hierarchy.

This adds a serialVersionUID to InstantSerializer, to ensure that it
doesn't change from removing the canEqual() method.

Plus we also remove canEqual() from PojoField, where it is not needed.
@aljoscha aljoscha force-pushed the drop-can-equal-serializer branch 2 times, most recently from b62ece3 to dc17d83 Compare February 19, 2019 12:53
@tzulitai
Copy link
Contributor

LGTM 👍

@aljoscha
Copy link
Contributor Author

Merged. Thanks for the reviews!

@aljoscha aljoscha closed this Feb 20, 2019
@aljoscha aljoscha deleted the drop-can-equal-serializer branch February 20, 2019 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants