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.

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

make recommendation service optional #1332

Merged
merged 6 commits into from
Dec 1, 2022
Merged

Conversation

katzio
Copy link
Contributor

@katzio katzio commented Dec 1, 2022

Background

make recommendation service optional like ad

Fixes

Change Summary

Frontend template already supports missing recommendations, removed renderHTTPError but kept logs

Additional Notes

should I add (error) log to placeOrderHandler?

Testing Procedure

Manually: turn off recommendations pod

Related PRs or Issues

addressing #1165

@katzio katzio requested a review from a team as a code owner December 1, 2022 09:27
@mathieu-benoit
Copy link
Contributor

Thanks for taking to initiative and opportunity to fix this, @katzio. Much appreciated!

Copy link
Contributor

@mathieu-benoit mathieu-benoit left a comment

Choose a reason for hiding this comment

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

I haven't tested myself, but this LGTM!

The associated application deployed (with recommendationservice working so not testing this PR per say) is working as expected: http://34.136.184.173/.

We want the same behavior as adservice, and I can see that you did the exact same code like here https://github.com/GoogleCloudPlatform/microservices-demo/blob/main/src/frontend/handlers.go#L432.

Thanks again!

Before merging, let's have a final approval by @NimJay on this.

@mathieu-benoit mathieu-benoit linked an issue Dec 1, 2022 that may be closed by this pull request
@mathieu-benoit
Copy link
Contributor

mathieu-benoit commented Dec 1, 2022

Actually, I just tested in our test environment (for this PR), and we could see that there is no error anymore on the Product page if recommendationservice is scaled to 0 pods (recommendationservice | OK | Deployment | 0/0), see screenshot.
image

Copy link
Collaborator

@NimJay NimJay left a comment

Choose a reason for hiding this comment

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

@katzio, thanks so much for looking into this and implementing it.
I didn't think the change would be this simple.
This is great!

@mathieu-benoit, thank you for taking down the recommendationservice of the staging deployment. It worked!

Approved. Merging!

@NimJay NimJay merged commit dfc78b9 into GoogleCloudPlatform:main Dec 1, 2022
D-Mwanth pushed a commit to D-Mwanth/microservices-demo that referenced this pull request Mar 6, 2024
* support missing recommendation service

* support in view cart

* lint

* improve comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make recommendation service optional like ad
3 participants