Skip to content

Simplify registry quickstart#327

Merged
wtrocki merged 10 commits intoredhat-developer:mainfrom
carlesarnal:simplify-registry-quickstart
Oct 4, 2021
Merged

Simplify registry quickstart#327
wtrocki merged 10 commits intoredhat-developer:mainfrom
carlesarnal:simplify-registry-quickstart

Conversation

@carlesarnal
Copy link
Collaborator

No description provided.

@carlesarnal
Copy link
Collaborator Author

cc @bibryam.

@carlesarnal
Copy link
Collaborator Author

@wtrocki this simplifies the service registry quickstart to only have one producer and one consumer instead of having two processes both producing and consuming.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 4, 2021

@carlesarnal Happy to help with review. Was this discussed with @bibryam just to make sure that we have approval on how this aligns with the guide?

@carlesarnal
Copy link
Collaborator Author

Yes, this was explicitly requested by @bibryam so we can simplify the instructions in the guide.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 4, 2021

Perfecto. On this now!

@@ -0,0 +1,143 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Question]
What is the benefit of having separate consumer and producer if we run them separately.
From configuration point of view we can then have single application.properties etc.

That will also simplify our SBO integration in later phase.
No strong opinion on this but considering couple factors:

  • Maintenance of the example/quarkus version updates will be easier
  • Testing and automation
  • Less code to maintain and chance to diverge.
  • More aligned with Kafka example as this is single module

Drawbacks:

  • Obviously not canonically correct (most the apps doesn't produce and consume at the same time)
  • Less of quickstart for users (producting/consuming flags will be mixed together)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the idea behind this is exactly what you mention as a drawback, to give users the simplest but realistic quickstart. As you said, in a real environment the producer and the consumer will be different applications. I don't have a strong opinion here either. @bibryam thoughts?

Copy link
Collaborator

@wtrocki wtrocki Oct 4, 2021

Choose a reason for hiding this comment

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

I do have strong opinion as my point of view is global (considering current and future services) +
maintenance considerations so please feel free to ignore me but..

Realistic quickstart drawback can be addressed by comments in application properties.
Having all config visible in a single property file is actually saving a lots of time (and guide steps)
There is also the second argument which is eventual confusion - we have 2 apps but they run as a single process that might also make the openshift deployment (SBO) guide a little bit confusing.

Using single module will cut some boilerplate code. It will also allow us to build up continuous tutorial - like Kafka guide that we have now can be adjusted to service registry scenario and that will require making changes in Kafka guide itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I understand your points and I think they make sense, I will leave the decision to Bilgin. @bibryam What do you prefer?

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 4, 2021

I have reviewed it locally. Good go merge from my point of view. Suggestions around making it single module are optional.

@bibryam
Copy link
Contributor

bibryam commented Oct 4, 2021

@wtrocki when I requested these changes to the quickstart, and included you and @smccarthy-ie in the email to explain why.
I prefer to have 2 processes as that explains better the reason for using RHOSR in a Kafka applicaiton. SR can help prevent changes that producer is making to the schema and not break the consumers. If producer and consumer are in the same process, there is no need for SR at all.

I trust your judgement that will be harder later, especially for SBO example, but I need to see that and see how SBO will fit into our example. I need to investigate that, but I'm guessing I expect two have 2 containers running producer and consumer separately in that scenario. That is what maps to the 2 processes running locally right now. We need to discuss this further, but let's proceed with the example as it is.

@bibryam
Copy link
Contributor

bibryam commented Oct 4, 2021

@smccarthy-ie how do you want to proceed with updating the draft based on this new example? This should simplify it. Happy to review once you have gone through it? Or let me know if you prefer me to do the changes first.

@wtrocki
Copy link
Collaborator

wtrocki commented Oct 4, 2021

Agreed! let's merge all changes! No need to split right now.
We can incorporate SBO without issues

@wtrocki wtrocki closed this Oct 4, 2021
@wtrocki wtrocki reopened this Oct 4, 2021
@wtrocki wtrocki merged commit 72792ef into redhat-developer:main Oct 4, 2021
@smccarthy-ie
Copy link
Contributor

smccarthy-ie commented Oct 4, 2021

@bibryam @wtrocki @carlesarnal I can update the latest draft docs based on these changes to the code and will ping you all on the PR for technical review. Thanks!

smccarthy-ie pushed a commit to smccarthy-ie/app-services-guides that referenced this pull request Oct 15, 2021
smccarthy-ie added a commit that referenced this pull request Oct 15, 2021
* clean up draft of service registry quarkus quick start from bilgin

* update section IDs to fix quick start build errors, minor edits

* add changes to simplify code example in PR 327

* add final step to view generated schemas

* clarify separate consumer and producer processes

* pick up new attributes

* adding technical review feedback from carles and wojciech

* adding technical review feedback from carles and wojciech

* add OAUTH_SERVER_URL and OAUTH_REALM env vars required for now until quarkus is updated

* add correct SR icon, QE and peer review feedback

* regen attributes for new guides

* regen attributes for new cli guides

* Simplify registry quickstart (#327)

* Service registry RHOAS CLI (#324)

* docs: initial version of the guide

* fix: restructure tutorial content for better alignment for RHOAS CLI

* docs: service registry cli guide

Signed-off-by: prmellor <[email protected]>

* docs: service registry cli guide

Signed-off-by: prmellor <[email protected]>

* fix: remove confusing sentence

Co-authored-by: PaulRMellor <[email protected]>

* docs: add abstract to procedures

Signed-off-by: prmellor <[email protected]>

* fix: build issue

* docs: service registry instance without defaults

Signed-off-by: prmellor <[email protected]>

* fix: add mixxing version from consumer config

* docs: remove angle brackets from example

Signed-off-by: prmellor <[email protected]>

* docs: change keys

Signed-off-by: prmellor <[email protected]>

* fix: another duplicated key

* Update rhoas-cli-kafka/README.adoc

* Update rhoas-cli-service-registry/README.adoc

Co-authored-by: PaulRMellor <[email protected]>

* fix: build issue

* docs: review edits BI

Signed-off-by: prmellor <[email protected]>

* docs: review edits WT for name command

Signed-off-by: prmellor <[email protected]>

Co-authored-by: Wojciech Trocki <[email protected]>

* docs: updated readme with new CLI doc (#329)

Signed-off-by: prmellor <[email protected]>

* docs: cli guide mod doc fix (#330)

Signed-off-by: prmellor <[email protected]>

* Add basic metrics section in getting started QS. (#332)

* Add basic metrics section in getting started QS.

* SME review from Duncan

* Peer tweaks

* docs: fix links in CLI guides (#334)

* docs: fix links in CLI guides

Signed-off-by: prmellor <[email protected]>

* cli command updates

Signed-off-by: prmellor <[email protected]>

* review edits RK

Signed-off-by: prmellor <[email protected]>

* return wget to step

Signed-off-by: prmellor <[email protected]>

* Add access mgmt content. (#325)

* fix: ./kafka-console-consumer.sh cmd line (#337)

Tested with Kafka 2.8.1 and the correct option is:
```
--consumer.config <String: config file>  Consumer config properties file. Note  
                                           that [consumer-property] takes       
                                           precedence over this config.    
```

* Remove name from service account commands (#336)

* Rethinking metrics approach (#339)

* Rethinking metrics approach

* Tweak metrics section

* John feedback

* Added missing dash to kafka-console-consumer command. (#335)

* Fix the pom.xml files for quarkus-service-registry-quickstart (#343)

* Update the url for Service Registry core REST API Reference and Kafka Service Fleet Manager API Reference (#341)

* Add placeholder values registry quickstart (#333)

* Check installation and version of rhoas CLI on Windows (#338)

* fix: add kafka access management for rhoas cli (#342)

* fix: add kafka acl commands

* fix: add note about cli reference doc

* Update rhoas-cli-kafka/README.adoc

Co-authored-by: PaulRMellor <[email protected]>

* Update rhoas-cli-kafka/README.adoc

Co-authored-by: Ramakrishna Pattnaik <[email protected]>

* Update rhoas-cli-kafka/README.adoc

Co-authored-by: PaulRMellor <[email protected]>

* Update rhoas-cli-kafka/README.adoc

Co-authored-by: PaulRMellor <[email protected]>

* Apply suggestions from code review

Co-authored-by: PaulRMellor <[email protected]>

* Update README.adoc

* fix: add acl bullet

* Update rhoas-cli-kafka/README.adoc

Co-authored-by: PaulRMellor <[email protected]>

* Update rhoas-cli-kafka/README.adoc

* Update rhoas-cli-kafka/README.adoc

Co-authored-by: Ramakrishna Pattnaik <[email protected]>

* Update rhoas-cli-kafka/README.adoc

* Update rhoas-cli-kafka/README.adoc

Co-authored-by: PaulRMellor <[email protected]>
Co-authored-by: Ramakrishna Pattnaik <[email protected]>

* Add missing links to getting started (#347)

* rebase for recently added new guides

Co-authored-by: Carles Arnal <[email protected]>
Co-authored-by: PaulRMellor <[email protected]>
Co-authored-by: Wojciech Trocki <[email protected]>
Co-authored-by: Stetson Robinson <[email protected]>
Co-authored-by: Davide Bizzarri <[email protected]>
Co-authored-by: Enda <[email protected]>
Co-authored-by: Duncan Doyle <[email protected]>
Co-authored-by: Eric Wittmann <[email protected]>
Co-authored-by: HemaHG <[email protected]>
Co-authored-by: Carles Arnal <[email protected]>
Co-authored-by: rkubis <[email protected]>
Co-authored-by: Ramakrishna Pattnaik <[email protected]>
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.

4 participants