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

Jump to bottom

core: Fix CallOptions 'wait for ready' and toString() #1918

Merged
merged 1 commit into from Jun 13, 2016
Merged

core: Fix CallOptions 'wait for ready' and toString() #1918

merged 1 commit into from Jun 13, 2016

Conversation

buchgr
Copy link
Collaborator

@buchgr buchgr commented Jun 11, 2016

No description provided.

@buchgr
Copy link
Collaborator Author

buchgr commented Jun 11, 2016

test failure seems unrelated.

io.grpc.DeadlineTest > defaultTickerIsSystemTicker[0] FAILED
    java.lang.AssertionError: <-23867801 ns from now> and <0 ns from now> should have been within <20000000ns> of each other
        at com.google.common.truth.FailureStrategy.fail(FailureStrategy.java:24)
        at com.google.common.truth.FailureStrategy.fail(FailureStrategy.java:20)
        at com.google.common.truth.Subject.failWithRawMessage(Subject.java:381)
        at io.grpc.testing.DeadlineSubject.access$400(DeadlineSubject.java:52)
        at io.grpc.testing.DeadlineSubject$1.of(DeadlineSubject.java:81)
        at io.grpc.DeadlineTest.defaultTickerIsSystemTicker(DeadlineTest.java:84)

@dapengzhang0
Copy link
Member

oh good catch:)

dapengzhang0
dapengzhang0 reviewed Jun 11, 2016
View reviewed changes
core/src/main/java/io/grpc/CallOptions.java
@@ -370,7 +371,7 @@ public String toString() {
toStringHelper.add("affinity", affinity);
toStringHelper.add("executor", executor != null ? executor.getClass() : null);
toStringHelper.add("compressorName", compressorName);
toStringHelper.add("customOptions", Arrays.toString(customOptions));
toStringHelper.add("customOptions", Arrays.deepToString(customOptions));
Copy link
Member

Choose a reason for hiding this comment

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

If we try
CallOptions.Default.withOption(option1, "v1").withOption(option1,"v2").toString()
(overwriting the same key option1),
it outputs
... customOptions=[[option1, v2], [option1, v1]], ...
Should we re-implement withOption() so that toString() can look nicer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dapengzhang0 Hmm. I assume it was implemented so that withOption doesn't have to always iterate through the whole array, and that it's probably unlikely that the same Key gets overwritten several times. Not sure whether this is a good tradeoff or premature optimization. I would assume that it's very fast to search through the array, as it's likely very small and search would just have to do == comparison. Maybe @elandau can comment on the implementation. I can't find any dicussion about it on the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dapengzhang0 I opened a PR that changes withOption accordingly: #1919

@buchgr buchgr mentioned this pull request Jun 12, 2016
Merged
@dapengzhang0
Copy link
Member

dapengzhang0 commented Jun 12, 2016 •
edited

I don't think we need introduce a benchmark for customOptions for now as it is not performance critical and I can't think of any usecase that is heavyduty on customOptions or changes existing customOptions very frequently.

dapengzhang0
dapengzhang0 reviewed Jun 12, 2016
View reviewed changes
core/src/main/java/io/grpc/CallOptions.java
@@ -295,22 +295,31 @@ public String toString() {
}

/**
* Set a custom option. Any existing value for the key overridden.
*
* Set a custom option. Any existing value for the key is overwritten.
Copy link
Member

Choose a reason for hiding this comment

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

Set -> Sets

@buchgr
Copy link
Collaborator Author

buchgr commented Jun 13, 2016 •
edited

@dapengzhang0 I think a benchmark is not strictly necessary, but I was curious about the performance impact of the additional loop. Seems like it's mostly the copying that takes the time.

Also I didn't even mean to add it in this PR. I added the commit by mistake. I removed it again.

@buchgr buchgr force-pushed the foobar branch 2 times, most recently from 26ee37b to 072e99e Compare June 13, 2016 14:56
@dapengzhang0
Copy link
Member

@buchgr Are you adding re-impl of withOptions in this PR or another one?

@buchgr
Copy link
Collaborator Author

buchgr commented Jun 13, 2016

@dapengzhang0 it's in #1919 ... I accidentially added the commit here.

@dapengzhang0
Copy link
Member

just one suggestion: amend the commit with a shorter title and put details in description lines before you merge, apart from that LGTM.
(same for other commits of yours:)

@buchgr
core: Fix CallOptions 'wait for ready' and toString()
c61083d 
- CallOptions 'wait for ready' was not passed between with*() calls and therefore
it was not possible to enable it via a stub.

- Properly format custom call options in toString() output.
@buchgr
Copy link
Collaborator Author

buchgr commented Jun 13, 2016

@dapengzhang0 I made the commit msg more clear. thanks!

@dapengzhang0 dapengzhang0 changed the title core: For CallOptions, fix 1) 'wait for ready' not passed between wit… core: Fix CallOptions 'wait for ready' and toString() Jun 13, 2016
@dapengzhang0
Copy link
Member

thanks

@buchgr buchgr merged commit 4b127f2 into grpc:master Jun 13, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@dapengzhang0 dapengzhang0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@buchgr @dapengzhang0