[testing]: Add "orca_per_rpc" test case#32524
Conversation
11f41fc to
9778645
Compare
markdroth
left a comment
There was a problem hiding this comment.
Thanks for doing this. This looks really good overall!
My main comment is the one about exposing the type of the map to store the backend metrics in. I think we can just do that internally inside of InteropClient, so that it doesn't need to be exposed to callers.
Please let me know if you have any questions. Thanks!
eugeneo
left a comment
There was a problem hiding this comment.
Thank you for the review. Please take another look.
858a67f to
30b29b5
Compare
|
This was an extensive change. I did a full reset to revert the files that no longer need to be changed. Please take another look. I don't think failed checks should block the review at this point. |
markdroth
left a comment
There was a problem hiding this comment.
The new LB policy looks great!
Just one substantive comment here, which is the one about simplifying the way that channel args are passed through the interop code. The current approach seems more complicated than it needs to be, and it took me a while to understand.
Please let me know if you have any questions. Thanks!
eugeneo
left a comment
There was a problem hiding this comment.
Done, please take another look.
eugeneo
left a comment
There was a problem hiding this comment.
Thank you for the review. I addressed the comments. PTAL.
markdroth
left a comment
There was a problem hiding this comment.
This looks really good!
Only minor comments remaining; feel free to merge after addressing.
Please let me know if you have any questions. Thanks!
| } | ||
| return CreateChannelForTestCase(test_case, std::move(factories), | ||
| std::move(arguments)); | ||
| arguments); |
There was a problem hiding this comment.
This should still use std::move().
There was a problem hiding this comment.
clang-tidy does not allow it...
test/cpp/interop/client.cc:231:41: error: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg,-warnings-as-errors]
std::move(arguments));
^~~~~~~~~~ ~
Suppressed 9286 warnings (9210 in non-user code, 76 NOLINT).
There was a problem hiding this comment.
Ah, I didn't realize that CreateChannelForTestCase() takes the argument as a const reference. It might make sense to change it to do so, but the way you have it now is fine too.
| } | ||
| return CreateChannelForTestCase(test_case, std::move(factories), | ||
| std::move(arguments)); | ||
| arguments); |
There was a problem hiding this comment.
Ah, I didn't realize that CreateChannelForTestCase() takes the argument as a const reference. It might make sense to change it to do so, but the way you have it now is fine too.
|
@eugeneo Hi, this PR might have broken the observability interop test. Those tests aren't ready to be run as PR tests yet so there wasn't a signal there. I noticed that the observability tests (master tests running every 8 hours) start failing after this PR is merged. Here's the logs that shows the The observability tests also use the interop server/client. I also see you make some changes to Do you mind taking a look? Hopefully it's something obvious. If it's not, I can help set up some way for you to reproduce this. It's basically just to If you do have a fix, I can help validate it against the observability interop test quickly. Thanks! |
Now `messages.proto` requires `xds/v3/orca_load_report.proto`. The dependency introduced in #32524. ref b/273575071
|
It is fairly obvious. I will try to fix it.
…On Tue, Mar 14, 2023 at 6:13 PM Stanley Cheung ***@***.***> wrote:
@eugeneo <https://github.com/eugeneo> Hi, this PR might have broken the
observability interop test. Those tests are ready to be run as PR tests yet
so there wasn't a signal there. I noticed that the observability tests
start failing after this PR is merged. Here's the logs that fails:
https://fusion2.corp.google.com/ci/kokoro/prod:grpc-gcp%2Ftools%2Fobservability%2Fmaster%2Fcontinuous_cpp/activity/f1566fe2-1017-4514-b828-c1c28be2ebf2/invocations/f1566fe2-1017-4514-b828-c1c28be2ebf2/targets/integration%2Fcpp:cpp:test_monitoring_basic/log
The observability tests also uses the interop server/client. I also see
you make some changes to observability_client.cc as well.
Do you mind taking a look? Hopefully it's something obvious. If it's not,
I can help set up some way for you to reproduce this. It's basically just
docker pull an image from gcr.io, set some environment variables and run
a docker run command.
Thanks.
—
Reply to this email directly, view it on GitHub
<#32524 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACGJLINVX3ZYKUUAOMRJWLW4EJSJANCNFSM6AAAAAAVNWI2SQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Thanks! Let me know if you have a PR, I can run it through. |
|
@stanley-cheung #32630 is in review now. |
|
Thanks, I just kicked off an observability interop test run. Will report back. Thanks! |
|
That PR did not build, sorry. Please wait for checks to pass. Thanks! |
|
Ah, cool, will wait |
|
@stanley-cheung that PR was checked in. |
|
@eugeneo That fixed the issue! Thanks a lot! |
This reverts commit c62ecd5.
…#32619) Now `messages.proto` requires `xds/v3/orca_load_report.proto`. The dependency introduced in grpc#32524. ref b/273575071
Now `messages.proto` requires `xds/v3/orca_load_report.proto`. The dependency introduced in #32524. ref b/273575071
Now `messages.proto` requires `xds/v3/orca_load_report.proto`. The dependency introduced in grpc/grpc#32524. ref b/273575071
No description provided.