Skip to content

Bookinfo app add MongoDB back end for ratings#634

Merged
rshriram merged 31 commits intoistio:masterfrom
colabsaumoh:bookinfo-mongodb
Sep 8, 2017
Merged

Bookinfo app add MongoDB back end for ratings#634
rshriram merged 31 commits intoistio:masterfrom
colabsaumoh:bookinfo-mongodb

Conversation

@saumoh
Copy link
Copy Markdown
Contributor

@saumoh saumoh commented Aug 31, 2017

Release note:

Enhance the bookinfo app's ratings (v2) micro service to be able to use mongodb backend in addition to mysql.

@istio-testing
Copy link
Copy Markdown
Collaborator

Hi @saumoh. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: douglas-reid

Assign the PR to them by writing /assign @douglas-reid in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@saumoh
Copy link
Copy Markdown
Contributor Author

saumoh commented Aug 31, 2017

@rshriram Please review the changes for mongodb backend for ratings v2 service. I've tested the change e2e.sh (no-rbac-no-auth) on my local setup and it is fine.

Copy link
Copy Markdown
Member

@ldemailly ldemailly left a comment

Choose a reason for hiding this comment

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

please make the change additive - a new config/option to use mongodb but not remove mysql, we need mysql in the rawvm demo (and I'm sure a bunch of customer would be happy to see either one)

@ldemailly ldemailly added the do-not-merge Block automatic merging of a PR. label Sep 1, 2017
@ldemailly
Copy link
Copy Markdown
Member

/ok-to-test

@saumoh
Copy link
Copy Markdown
Contributor Author

saumoh commented Sep 1, 2017

i have two suggested alternatives to support both mongo and mysql:

  1. Have a new version of reviews (v4) or details (v2) use mongodb and ratings (v2) use mysql -which is what we have today.
  2. Have one version of ratings(v2) use mysql (as today) and ratings(v3) use mongodb.
    That way we have all the flexibility and can use routing-rules to try out all the desired combinations as well.
    wdyt?

@ldemailly
Copy link
Copy Markdown
Member

Code wise I think a flag/env var to select the DB backend makes the most sense

Yaml wise whichever is easiest for you but I don't think we need to have both mongodb and mysql in the same deployment - some sort of selection of either one is fine with me
(ie you kubectl either mongodb yaml or a mysql yaml and the code finds out or is pointed to the right one)

@ldemailly
Copy link
Copy Markdown
Member

Error on bluemix:

E0831 23:22:46.326121    4479 demo_test.go:132] Failed database routing! normal-user in v1. Error could not find glyphicon-star in response

@linsun
Copy link
Copy Markdown
Member

linsun commented Sep 1, 2017

+1 on supporting both mongo and mysql, an env var to allow user to specify would be great.

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Sep 1, 2017

This is no different from having mongodb from the very first PR. We can always have a custom version for the demo. But let's not unnecessarily complicate the tutorials and samples with too many options (MySQL adds no value over mongo). Please remove flags and other stuff and fully revert to mongo.

containers:
- name: ratings
image: quay.io/saurabh/examples-bookinfo-ratings-v2:v1
image: quay.io/saurabh/examples-bookinfo-ratings-mongo:v1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't change

@costinm
Copy link
Copy Markdown
Contributor

costinm commented Sep 1, 2017

Please don't remove mysql - we have worked on having an example with mysql, and it does add value - some companies use mysql, some use mongodb. If someone likes posgresql - it shouldn't remove mongodb and mysql.

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Sep 1, 2017 via email

@ldemailly
Copy link
Copy Markdown
Member

MySQL adds no value over mongo

if it's equivalent why does it bother you so much to keep the already working one, working

I am trying to help you here.

Not really no, you're trying to remove a working option for no reason (that I can understand)

It's really easy to make this change additive, easier than arguing about it endlessly - and would have been easier if you add not requested a replacement without consulting anyone, now mongodb is delayed but I'm sure it'll be in in time for some of the examples/tutorials later

@kyessenov
Copy link
Copy Markdown
Contributor

Envoy has a monitoring filter for mongodb but not mysql AFAIK. mongodb is strictly better in this regard.

@ldemailly
Copy link
Copy Markdown
Member

ldemailly commented Sep 1, 2017

nobody is saying to not have the option to use mongodb if they want to showcase special treatment in envoy (that we don't use in mixer afaik - specifically the statsd isn't working yet because it's using udp); just that 3 days before a key demo you don't remove something that already works and is relied upon by other people - feel free to have a 0.3 task to remove mysql later and we can argue the validity of that, but we will not remove it right now, there are way too many moving parts and things that might break already.

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Sep 2, 2017

Because it adds bloat and unnecessary code. because people like this app for its simplicity not its versatility. Because a demo is one time enemy and public docs live for much longer, and that like Greg, you can certainly afford to show the demo from your personal branch. I guess you won't though and will insist on demoing from master. I can probably state more reasons but I am too tired to reason with you yet again, with your constant ad hominem remarks sans valid reason. So feel free to do what you will.

@costinm
Copy link
Copy Markdown
Contributor

costinm commented Sep 2, 2017 via email

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Sep 3, 2017 via email

@saumoh
Copy link
Copy Markdown
Contributor Author

saumoh commented Sep 5, 2017

There are two requirements:

  1. A stable point in the repo which folks can dependably use to build their features on top off.
  2. A way to iterate existing features so that the repo can have features that are 'better' than whats already in.
    The way i've experienced these requirements being addressed in other open source projects is to have a 'next' branch. Changes such as this pr go into next branch. The repo owner decides when the next change set gets into master.
    That way we collectively can have a stable master and also have a way for the repo to keep moving forward as well.
    I guess this comes down to building a process that can satisfy these requirements. If such a process is seen as useful then I'd be happy to work with the repo owner(s) to make it more concrete.
    wdyt?

@costinm
Copy link
Copy Markdown
Contributor

costinm commented Sep 5, 2017 via email

Copy link
Copy Markdown
Member

@ldemailly ldemailly left a comment

Choose a reason for hiding this comment

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

lgtm
but if you can please try to have a test for both code path (by switching the db/yaml setup) that will help ensure it doesn't rot/break

also let's have the tests pass before merging

@ldemailly ldemailly added approved and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. do-not-merge Block automatic merging of a PR. labels Sep 7, 2017
@googlebot googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Sep 7, 2017
@rshriram rshriram added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Sep 7, 2017
@istio-testing
Copy link
Copy Markdown
Collaborator

@saumoh: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/e2e-suite-no_rbac-auth.sh da4e6a5 link /test e2e-suite-no_rbac-auth
prow/e2e-suite-no_rbac-no_auth.sh e8d6d5b link /test e2e-suite-no_rbac-no_auth
prow/e2e-suite-rbac-auth.sh 0a6f619 link /test e2e-suite-rbac-auth
Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Sep 8, 2017 via email

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Sep 8, 2017

Logs collected. Basic tests pass. Merging.

@rshriram rshriram merged commit 22014c8 into istio:master Sep 8, 2017
@rshriram rshriram mentioned this pull request Sep 8, 2017
mandarjog pushed a commit to mandarjog/istio that referenced this pull request Oct 30, 2017
Add benchmark test for code between adapterManager dispatch and before adapters are invoked (through the aspect managers).

Includes 4 tests:

single aspect with simple expressions,
50 aspects with simple expressions,
single aspect with complex expressions,
50 aspects with complex expressions,


Former-commit-id: 974543c9f1d219945b7d3ff10964c1b1aea5d354
rshriram pushed a commit that referenced this pull request Oct 30, 2017
* Add mongodb container for ratings.

* Ratings-v2 service use mongodb instead of mysql.

* Bookinfo use mongodb instead of mysql.

* Change tests to use the db yamls.

* Revert "Add mongodb container for ratings."

This reverts commit 94f3a5b.

* Revert "Ratings-v2 service use mongodb instead of mysql."

This reverts commit 13a15c6.

* Revert "Bookinfo use mongodb instead of mysql."

This reverts commit 5aec460.

* Revert "Change tests to use the db yamls."

This reverts commit da4e6a5.

* Remove reference to price table in mysql db.

* Demo test changes for new db files.

* Add mongodb as the backend db for ratings.

* Ratings service dependent on mongodb and include options to support mysql also.

* Bookinfo yamls for db use.

* docker build changes for mongdb image.

* Remove mysql route-rule.

* Fix docker image names.

* Rename service port to mongo.

* Update bookinfo-db.yaml

* Update bookinfo-mysql.yaml

* Update bookinfo-ratings-v2.yaml

* Use istio images.

* Move ratings-db rules under the rules directory. as thats the directory used by e2e tests.

* Make db routing rule compatible with kubectl command.

* Create ratings-v2 manifest with mysql settings.


Former-commit-id: 22014c8
mandarjog pushed a commit that referenced this pull request Oct 31, 2017
Add benchmark test for code between adapterManager dispatch and before adapters are invoked (through the aspect managers).

Includes 4 tests:

single aspect with simple expressions,
50 aspects with simple expressions,
single aspect with complex expressions,
50 aspects with complex expressions,


Former-commit-id: 862182a5f716db0be20003d0f7661d7f597ce755
vbatts pushed a commit to vbatts/istio that referenced this pull request Oct 31, 2017
* Add mongodb container for ratings.

* Ratings-v2 service use mongodb instead of mysql.

* Bookinfo use mongodb instead of mysql.

* Change tests to use the db yamls.

* Revert "Add mongodb container for ratings."

This reverts commit 0af22f0c016f7447bff7bffd8126228eb38dd5ac [formerly 94f3a5b].

* Revert "Ratings-v2 service use mongodb instead of mysql."

This reverts commit 49b51b0d4bfa3c7888fe7c100b4c918f62e19596 [formerly 13a15c6].

* Revert "Bookinfo use mongodb instead of mysql."

This reverts commit 134c3091f4ea4c8a723911d5265ec5919e0f14b4 [formerly 5aec460].

* Revert "Change tests to use the db yamls."

This reverts commit 81da2515d81684f2d57903e8192cc07003175b0d [formerly da4e6a5].

* Remove reference to price table in mysql db.

* Demo test changes for new db files.

* Add mongodb as the backend db for ratings.

* Ratings service dependent on mongodb and include options to support mysql also.

* Bookinfo yamls for db use.

* docker build changes for mongdb image.

* Remove mysql route-rule.

* Fix docker image names.

* Rename service port to mongo.

* Update bookinfo-db.yaml

* Update bookinfo-mysql.yaml

* Update bookinfo-ratings-v2.yaml

* Use istio images.

* Move ratings-db rules under the rules directory. as thats the directory used by e2e tests.

* Make db routing rule compatible with kubectl command.

* Create ratings-v2 manifest with mysql settings.


Former-commit-id: 22014c8
mandarjog pushed a commit that referenced this pull request Nov 2, 2017
* Add mongodb container for ratings.

* Ratings-v2 service use mongodb instead of mysql.

* Bookinfo use mongodb instead of mysql.

* Change tests to use the db yamls.

* Revert "Add mongodb container for ratings."

This reverts commit 0af22f0c016f7447bff7bffd8126228eb38dd5ac [formerly 94f3a5b].

* Revert "Ratings-v2 service use mongodb instead of mysql."

This reverts commit 49b51b0d4bfa3c7888fe7c100b4c918f62e19596 [formerly 13a15c6].

* Revert "Bookinfo use mongodb instead of mysql."

This reverts commit 134c3091f4ea4c8a723911d5265ec5919e0f14b4 [formerly 5aec460].

* Revert "Change tests to use the db yamls."

This reverts commit 81da2515d81684f2d57903e8192cc07003175b0d [formerly da4e6a5].

* Remove reference to price table in mysql db.

* Demo test changes for new db files.

* Add mongodb as the backend db for ratings.

* Ratings service dependent on mongodb and include options to support mysql also.

* Bookinfo yamls for db use.

* docker build changes for mongdb image.

* Remove mysql route-rule.

* Fix docker image names.

* Rename service port to mongo.

* Update bookinfo-db.yaml

* Update bookinfo-mysql.yaml

* Update bookinfo-ratings-v2.yaml

* Use istio images.

* Move ratings-db rules under the rules directory. as thats the directory used by e2e tests.

* Make db routing rule compatible with kubectl command.

* Create ratings-v2 manifest with mysql settings.


Former-commit-id: 22014c8
howardjohn pushed a commit to howardjohn/istio that referenced this pull request Jan 12, 2020
howardjohn pushed a commit to howardjohn/istio that referenced this pull request Jan 12, 2020
* Update common files

* Use same image as CI so generators work correctly
0x01001011 pushed a commit to thedemodrive/istio that referenced this pull request Jul 16, 2020
* Document subset without labels

* regen

* fix typo
luksa pushed a commit to luksa/istio that referenced this pull request Oct 7, 2022
Co-authored-by: maistra-bot <null>
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.

10 participants