api: fix bugs of missing to copy customOptions when converting to a new builder, also trash hashCode/equals by voidzcy · Pull Request #5647 · grpc/grpc-java

voidzcy · 2019-04-29T22:17:05Z

This is the second (last) thing I missed in #5640 .

dapengzhang0 · 2019-04-29T22:29:49Z


+      private <T> Builder copyCustomOptions(Object[][] options) {
+        customOptions = new Object[options.length][2];
+        System.arraycopy(customOptions, 0, options, 0, options.length);


No. Options are intended to be mutable. So keys and values are compared by identity.

I'm confused. You used deepEquals for comparison which is not compared by identity. And here, coping builder is not for comparison.

What does deep copy even mean here? The keys are immutable and the objects are arbitrary. How could we do a deep copy?

@dapengzhang0
In equals method, I should do unordered identity-based equal instead of deepEquals. Keys and values should be used as identity-based.

I can agree using shallow copy for copying builder, but equals and hashCode should also be shallow for the same reason.

ejona86
What does deep copy even mean here? The keys are immutable and the objects are arbitrary. How could we do a deep copy?

I was just glancing at the code and noticed the inconsistency between copy and equals/hashCode, didn't know about the mutability detail.

I don't understand what equals has to do with copy. We can use equals() even if we don't deep copy.

Keys should be identity-based. It is unclear if values should. I'd have expected to choose identity if we don't care about equals for custom options and use equals otherwise. This is why I hate defining equals/hashCode...

lol. This is very subtle. Strings are always identity-based if we use with literals, while boxed numbers are not. This might bring us some trouble when using this. But I would agree that leaving values identity-based is better.

Okay. I'm not hearing anything a reason for using identity for values. Let's use equals().

I'm still +1 for not defining equals though. This doesn't seem like something that anyone would even use during tests. With the stateListener, it is highly, highly unlikely to be equal with another instance.

ejona86 · 2019-04-29T22:41:54Z

      return Objects.equal(addrs, that.addrs) && Objects.equal(attrs, that.attrs)
-          && Objects.equal(stateListener, that.stateListener);
+          && Objects.equal(stateListener, that.stateListener)
+          && Arrays.deepEquals(customOptions, that.customOptions);


It's unclear what equality really means, but depending on the use-case the order that custom options were set should not matter, but here it does. (see also: hash code)

Huh. Shouldn't this also apply for EquivalentAddressGroup?

Order matters for sockets in EAG. It doesn't matter in Attributes. And in attributes we ignore order.

It looks like we've not defined equals for CallOptions, not Context.

dapengzhang0 · 2019-04-30T00:23:48Z

+        return false;
+      }
+      if (customOptions.length > 0) {
+        Map<Object, Object> thisOptionMap = new HashMap<>();


Set<?> thisOptionSet = new HashSet<>(Arrays.asList((Object[]) customOptions)); Set<?> thatOptionSet = new HashSet<>(Arrays.asList((Object[]) that.customOptions));

might be better?

Never mind, this is not the intended behavior.

…Buider, hashCode, equals

ejona86 · 2019-04-30T00:52:40Z

@voidzcy, please avoid force-pushing during review, as it means we can't see what changed since we looked at it last.

ejona86

Discussing with @zhangkun83, and we can't come up with a time equals/hashCode would actually be useful, even in a test. Unless there's a good reason to have them, we should trash equals/hashCode instead of "fixing" them.

ejona86

Note: I only realized now that we are using a 2d array for options. Although it doesn't matter, generally we would prefer a 1d array. I may now better understand comments from @dapengzhang0 about deep copies.

voidzcy requested review from dapengzhang0, ejona86 and zhangkun83 April 29, 2019 22:17

voidzcy changed the title ~~api: fix bugs of missing out customOptions in CreateSubchannelArgs toBuider, hashCode, equals~~ api: fix bugs of missing out customOptions in CreateSubchannelArgs toBuilder, hashCode, equals Apr 29, 2019

zhangkun83 approved these changes Apr 29, 2019

View reviewed changes

dapengzhang0 reviewed Apr 29, 2019

View reviewed changes

ejona86 reviewed Apr 29, 2019

View reviewed changes

voidzcy force-pushed the bugfix/create_subchannel_args_to_builder_dropped_custom_options branch 3 times, most recently from b8707d8 to e756ea8 Compare April 30, 2019 00:02

voidzcy requested review from dapengzhang0 and ejona86 April 30, 2019 00:05

dapengzhang0 reviewed Apr 30, 2019

View reviewed changes

api: fix bugs of missing out customOptions in CreateSubchannelArgs to…

56a3967

…Buider, hashCode, equals

voidzcy force-pushed the bugfix/create_subchannel_args_to_builder_dropped_custom_options branch from e756ea8 to 56a3967 Compare April 30, 2019 00:33

ejona86 reviewed Apr 30, 2019

View reviewed changes

voidzcy force-pushed the bugfix/create_subchannel_args_to_builder_dropped_custom_options branch from 75387f8 to e079575 Compare April 30, 2019 01:03

voidzcy changed the title ~~api: fix bugs of missing out customOptions in CreateSubchannelArgs toBuilder, hashCode, equals~~ api: trash equals and hashCode in CreateSubChannelArgs as they are problematic Apr 30, 2019

voidzcy force-pushed the bugfix/create_subchannel_args_to_builder_dropped_custom_options branch from e079575 to ae85e50 Compare April 30, 2019 01:08

ejona86 approved these changes Apr 30, 2019

View reviewed changes

trash equals/hashCode for CreateSubchannelArgs as they are problematic

8de6443

voidzcy force-pushed the bugfix/create_subchannel_args_to_builder_dropped_custom_options branch from ae85e50 to 8de6443 Compare April 30, 2019 01:13

voidzcy changed the title ~~api: trash equals and hashCode in CreateSubChannelArgs as they are problematic~~ api: fix bugs of missing to copy customOptions when converting to a new builder, also trash hashCode/equals Apr 30, 2019

voidzcy merged commit 44840fe into grpc:master Apr 30, 2019

lock Bot locked as resolved and limited conversation to collaborators Jul 29, 2019

Conversation

voidzcy commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voidzcy Apr 29, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voidzcy Apr 29, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dapengzhang0 Apr 30, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Apr 30, 2019

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

voidzcy Apr 29, 2019 •

edited

Loading

voidzcy Apr 29, 2019 •

edited

Loading

dapengzhang0 Apr 30, 2019 •

edited

Loading