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

Don't use pointers to interfaces #294

Merged
merged 4 commits into from
Sep 15, 2022
Merged

Don't use pointers to interfaces #294

merged 4 commits into from
Sep 15, 2022

Conversation

sethvargo
Copy link
Contributor

Depends on abcxyz/jvs#117 first

@sethvargo sethvargo requested review from a team, myurtoglu, sqin2019, capri-xiyue and yolocs and removed request for myurtoglu and sqin2019 September 15, 2022 00:40
@yolocs
Copy link
Contributor

yolocs commented Sep 15, 2022

Thanks!

@sethvargo
Copy link
Contributor Author

Tests seem to be failing because of #295

@yolocs
Copy link
Contributor

yolocs commented Sep 15, 2022

Tests seem to be failing because of #295

Thanks for the fix in the other PR.
But the error seems rather accidental.

Err while reading fibonacci stream: rpc error: code = ResourceExhausted desc = grpc: received message larger than max (1702000233 vs. 4194304)

Let me retry the test. Meantime let me investigate why the error happened in the first place.

@sethvargo sethvargo mentioned this pull request Sep 15, 2022
Closed
@yolocs
Copy link
Contributor

yolocs commented Sep 15, 2022

I found the root cause of the test failure. We pulled in a new version of JVS in which it changed the logic to use envconfig to set config defaults. But in the lumberjack code we didn't load the JVS client config in the right way so JVS client can't be created. The test failure was directly caused by the test app failing to start.

I will send a fix soon.

@yolocs
Copy link
Contributor

yolocs commented Sep 15, 2022

That said, I expect the test will still fail here :(

@yolocs yolocs merged commit 40a3a08 into main Sep 15, 2022
@yolocs yolocs deleted the sethvargo/ptr branch September 15, 2022 19:14
yolocs added a commit that referenced this pull request Jun 13, 2023
@sethvargo @yolocs
Don't use pointers to interfaces (#294)
534baf3 
Co-authored-by: Chen Shou <cshou@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mithu147 Mithu147 Mithu147 approved these changes

@capri-xiyue capri-xiyue capri-xiyue approved these changes

@myurtoglu myurtoglu myurtoglu approved these changes

@yolocs yolocs Awaiting requested review from yolocs

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sethvargo @yolocs @myurtoglu @capri-xiyue @Mithu147