Bookinfo app add MongoDB back end for ratings#634
Bookinfo app add MongoDB back end for ratings#634rshriram merged 31 commits intoistio:masterfrom colabsaumoh:bookinfo-mongodb
Conversation
|
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 I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:You can indicate your approval by writing |
|
@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. |
ldemailly
left a comment
There was a problem hiding this comment.
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)
|
/ok-to-test |
|
i have two suggested alternatives to support both mongo and mysql:
|
|
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 |
|
Error on bluemix: |
|
+1 on supporting both mongo and mysql, an env var to allow user to specify would be great. |
|
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 |
|
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. |
|
This is not about showing our stuff working with a particular database
technology. It is about showing that we can talk to tcp services outside
the mesh. Using mongo has the additional (cool factor) advantage that we
can show Envoy stats for these external services.
If we run behind a technology, we have to have services in 10s of
languages, database solutions (as "some" company might have implemented
their stuff in some language and db), and also 10s of messaging solutions.
I volunteer to change your install scripts to install mongo instead of
MySQL, if that's your main concern. It's two line change.
I am trying to help you here. The example we publish on our site will use
mongo (all in k8s or using hosted mongo), where we show end to end
visibility without Envoy on mongo server.
Having a hard dependency on MySQL implies that some specific tutorial will
depend on MySQL (not a smooth user experience). But if you insist that
MySQL is the only golden demo for VMs, then your call. You would have to
massage the writing in your section of docs to justify the database switch
and then redraw the diagrams, deal with changes, so on so forth.
Having a single db makes for a much more smoother experience.
On Fri, Sep 1, 2017 at 12:33 PM Costin Manolache ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AH0qd0bTwyDhm7Ea898TZNgAWpLdwH35ks5seDHDgaJpZM4PJgdd>
.
--
~shriram
|
if it's equivalent why does it bother you so much to keep the already working one, working
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 |
|
Envoy has a monitoring filter for mongodb but not mysql AFAIK. mongodb is strictly better in this regard. |
|
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. |
|
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. |
|
I think in an open source project it is very important to accept and
embrace the diversity of opinions and goals - I do agree mongodb
is cooler, easier to setup - and would make a better demo, and I'm looking
forward to use it.
But at the same time - mysql is also used in a lot of organizations, and is
an example that Istio supports the not-so-cool class
of SQL databases. I also agree it adds bloat and deps - but the purpose of
the app is showcase what Istio can do, it already
uses 5 different languages, so clearly 'simplicity' is not the driving
factor :-)
I've seen many similar debates in open source projects - things work better
if everyone agrees to respect other committers use cases
and goals instead of proving that one use case is more important and the
other should not be implemented.
I was not particularly happy with the initial choice of Mysql - for a SQL
database I would have used postgresql, and I also think that
a noSQL database would be cooler and better example :-), but there is no
value to argue to support only one, if one member of the
project feels showing Istio support for SQL - and mysql in particular - is
important enough there is no valid reason to reject the use
case.
Time also matters - if the argument to use only mongodb was made early in
the cycle, or before the mysql change was made -
and not a day before code freeze and after we spent time testing and
getting the demo work as was submitted - I think I
would have happily voted to remove or not start with mysql. Everyone is
happy to see Mongodb as an option - and I hope
more integrations will be showcased in future.
…On Fri, Sep 1, 2017 at 9:36 PM, Shriram Rajagopalan < ***@***.***> wrote:
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.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6kY568jpXh60ItpJgzXAgwX0MZiXks5seNtjgaJpZM4PJgdd>
.
|
|
Thanks. There are more rational and valid arguments than earlier comments.
If you feel strongly about MySQL, let's keep it, but it's your
responsibility to work them into the docs story in addition to mongo. More
responses inline.
On Sat, Sep 2, 2017 at 11:50 AM Costin Manolache <notifications@github.com>
wrote:
I think in an open source project it is very important to accept and
embrace the diversity of opinions and goals - I do agree mongodb
is cooler, easier to setup - and would make a better demo, and I'm looking
forward to use it.
It's not about ease of setup or use. It's purely to show that we can do
monitoring of database dependencies like mongo and redis (a key feature of
Envoy). And we would likely add more databases to Envoy in the future.
But at the same time - mysql is also used in a lot of organizations, and is
an example that Istio supports the not-so-cool class
of SQL databases.
Once again, we have nothing special in our code for sql support. It's just
plain tcp proxying. So, given the choice between tcp proxy for two
databases, I am merely proposing to pick the one for which Envoy has
monitoring support.
I also agree it adds bloat and deps - but the purpose of
the app is showcase what Istio can do, it already
uses 5 different languages, so clearly 'simplicity' is not the driving
factor :-)
Let me clarify. We do not have choices in the app today. It's a single
story. This x or y database thing creates branches in the story. Different
variants and that is typicallly not a great ux. Look at kube examples. One
db, one story. They don't have the ghost app in many dbs. But once again, I
am totally fine with this, as long as you take responsibility for properly
massaging the story flow when people step into your section of the docs. It
can be done, just that it is not clean. If done right, it fits in well.
Looking forward to the docs PRs. Fair warning, that I might be picky about
the way the text is written to make sure it flows smoothly. That is not a
sign of rejection, just trying to maintain our standards. I can help smooth
out once it's in decent form.
I've seen many similar debates in open source projects - things work better
if everyone agrees to respect other committers use cases
and goals instead of proving that one use case is more important and the
other should not be implemented.
I was not particularly happy with the initial choice of Mysql - for a SQL
database I would have used postgresql, and I also think that
a noSQL database would be cooler and better example :-), but there is no
value to argue to support only one, if one member of the
project feels showing Istio support for SQL - and mysql in particular - is
important enough there is no valid reason to reject the use
case.
See the argument about the tcp proxy. Also, someone else contributed this
code. If we wanted, we should write our own.
Time also matters - if the argument to use only mongodb was made early in
the cycle, or before the mysql change was made -
and not a day before code freeze and after we spent time testing and
getting the demo work as was submitted - I think I
would have happily voted to remove or not start with mysql. Everyone is
happy to see Mongodb as an option - and I hope
more integrations will be showcased in future.
On Fri, Sep 1, 2017 at 9:36 PM, Shriram Rajagopalan <
***@***.***> wrote:
> 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.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#634 (comment)>, or
mute
> the thread
> <
https://github.com/notifications/unsubscribe-auth/AAFI6kY568jpXh60ItpJgzXAgwX0MZiXks5seNtjgaJpZM4PJgdd
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AH0qdw-lKKFvyv-nt7Q2Y194Jd9RD8slks5seXlagaJpZM4PJgdd>
.
--
~shriram
|
|
There are two requirements:
|
|
On Sun, Sep 3, 2017 at 6:24 AM, Shriram Rajagopalan <
notifications@github.com> wrote:
Thanks. There are more rational and valid arguments than earlier comments.
If you feel strongly about MySQL, let's keep it, but it's your
responsibility to work them into the docs story in addition to mongo. More
responses inline.
On Sat, Sep 2, 2017 at 11:50 AM Costin Manolache ***@***.***
>
wrote:
> I think in an open source project it is very important to accept and
> embrace the diversity of opinions and goals - I do agree mongodb
> is cooler, easier to setup - and would make a better demo, and I'm
looking
> forward to use it.
It's not about ease of setup or use. It's purely to show that we can do
monitoring of database dependencies like mongo and redis (a key feature of
Envoy). And we would likely add more databases to Envoy in the future.
My concern with mongodb was also that we were not showing Envoy advanced
features - but this is addressed by your PR.
Hope Envoy will add similar support for postgresql and mysql (there are a
bunch
of other DB and FTS sharing the same the raw protocol ) :-).
>
>
> But at the same time - mysql is also used in a lot of organizations, and
is
> an example that Istio supports the not-so-cool class
> of SQL databases.
Once again, we have nothing special in our code for sql support. It's just
plain tcp proxying. So, given the choice between tcp proxy for two
databases, I am merely proposing to pick the one for which Envoy has
monitoring support.
+1, in context of latest PR mongodb is better for demo.
I also agree it adds bloat and deps - but the purpose of
> the app is showcase what Istio can do, it already
> uses 5 different languages, so clearly 'simplicity' is not the driving
> factor :-)
>
Let me clarify. We do not have choices in the app today. It's a single
story. This x or y database thing creates branches in the story. Different
variants and that is typicallly not a great ux. Look at kube examples. One
db, one story. They don't have the ghost app in many dbs. But once again, I
am totally fine with this, as long as you take responsibility for properly
massaging the story flow when people step into your section of the docs. It
can be done, just that it is not clean. If done right, it fits in well.
Looking forward to the docs PRs. Fair warning, that I might be picky about
the way the text is written to make sure it flows smoothly. That is not a
sign of rejection, just trying to maintain our standards. I can help smooth
out once it's in decent form.
Sounds fair - and in context of your PR adding explicit mongodb I agree it's
a better/cleaner story.
Costin
…
> I've seen many similar debates in open source projects - things work
better
> if everyone agrees to respect other committers use cases
> and goals instead of proving that one use case is more important and the
> other should not be implemented.
>
> I was not particularly happy with the initial choice of Mysql - for a SQL
> database I would have used postgresql, and I also think that
> a noSQL database would be cooler and better example :-), but there is no
> value to argue to support only one, if one member of the
> project feels showing Istio support for SQL - and mysql in particular -
is
> important enough there is no valid reason to reject the use
> case.
>
See the argument about the tcp proxy. Also, someone else contributed this
code. If we wanted, we should write our own.
> Time also matters - if the argument to use only mongodb was made early in
> the cycle, or before the mysql change was made -
> and not a day before code freeze and after we spent time testing and
> getting the demo work as was submitted - I think I
> would have happily voted to remove or not start with mysql. Everyone is
> happy to see Mongodb as an option - and I hope
> more integrations will be showcased in future.
>
>
>
>
>
>
> On Fri, Sep 1, 2017 at 9:36 PM, Shriram Rajagopalan <
> ***@***.***> wrote:
>
> > 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.
> >
> > —
> > You are receiving this because you commented.
> > Reply to this email directly, view it on GitHub
> > <#634 (comment)>, or
> mute
> > the thread
> > <
> https://github.com/notifications/unsubscribe-auth/
AAFI6kY568jpXh60ItpJgzXAgwX0MZiXks5seNtjgaJpZM4PJgdd
> >
> > .
> >
>
> —
> You are receiving this because you were mentioned.
>
>
> Reply to this email directly, view it on GitHub
> <#634 (comment)>, or
mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AH0qdw-lKKFvyv-
nt7Q2Y194Jd9RD8slks5seXlagaJpZM4PJgdd>
> .
>
--
~shriram
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6tf_u8q7hbFRP6QyLCxyPSlkMknkks5seqirgaJpZM4PJgdd>
.
|
ldemailly
left a comment
There was a problem hiding this comment.
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
|
@saumoh: The following tests failed, say
DetailsInstructions 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. |
|
Do not retest please
On Thu, Sep 7, 2017 at 8:59 PM istio-bot ***@***.***> wrote:
@saumoh <https://github.com/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
<da4e6a5>
link
<https://k8s-gubernator.appspot.com/build/istio-prow/pull/istio_istio/634/e2e-suite-no_rbac-auth/191/> /test
e2e-suite-no_rbac-auth
prow/e2e-suite-no_rbac-no_auth.sh e8d6d5b
<e8d6d5b>
link
<https://k8s-gubernator.appspot.com/build/istio-prow/pull/istio_istio/634/e2e-suite-no_rbac-no_auth/342/> /test
e2e-suite-no_rbac-no_auth
prow/e2e-suite-rbac-auth.sh 0a6f619
<0a6f619>
link
<https://k8s-gubernator.appspot.com/build/istio-prow/pull/istio_istio/634/e2e-suite-rbac-auth/341/> /test
e2e-suite-rbac-auth
Instructions for interacting with me using PR comments are available here
<https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>.
If you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
<https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:>
repository. I understand the commands that are listed here
<https://github.com/kubernetes/test-infra/blob/master/commands.md>.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#634 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AH0qd-ztguZcoyTEdOWUvYMn6NilXKE8ks5sgJGAgaJpZM4PJgdd>
.
--
~shriram
|
|
Logs collected. Basic tests pass. Merging. |
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
* 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
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
* 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
* 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
* Update common files * Use same image as CI so generators work correctly
* Document subset without labels * regen * fix typo
Co-authored-by: maistra-bot <null>
Release note: