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

closes #10 Display pets' visits on owner details screen #19

Merged
merged 1 commit into from
Dec 5, 2016

Conversation

mszarlinski
Copy link
Collaborator

Added example data aggregation from multiple backing services. @arey please review.

@arey
Copy link
Member

arey commented Nov 24, 2016

I Will check tomorrow

@arey
Copy link
Member

arey commented Nov 25, 2016

I like your changes but I've got a lot of questions:

  • The Api Gateway is not just an API. It now does data aggregation. We also could fetch visits from the Angular client side. What is the best in a micro services architecture?
  • We could see usage of a LoadBalanced RestTemplate. It’s a first step for using Hystrix and @EnableCircuitBreaker. Isn’t it?
  • You also introduce usage of named service (ie. http://customers-service/owners/{ownerId}). It’s great.
  • I am not familial with Lombok and syntax like this: @requiredargsconstructor(onConstructor = @__(@Autowired)). Do you really want to use Lombok?
  • Classes VisitDetails, PetDetails, OwnerDetails and PetType has been added. They duplicates Visit, Pet, Owner and PetType model classes. I know it’s a bad practice to share thoses classes between micro-services. But in the Api Gateway?
  • For each Pet, VisitsServiceClient does a REST call. ParallelStream is used. But for better performance, the visits-service may provides an API with takes a list of pets id. No ?

@mszarlinski
Copy link
Collaborator Author

Thanks for your questions, I will answer them one by one.

  • The Api Gateway is not just an API. It now does data aggregation. We also could fetch visits from the Angular client side. What is the best in a micro services architecture?

    • The question is what is Api Gateway responsible for - it is only a router or provides some services too. I must admit my implementation is based on Springbox sample application. Of course, we could do data aggregation on a client, but I think we should strive to hide internal microservices structure which can change over time whereas API remains stable. More on that topic is written here. I think it is a good topic to discuss.
  • We could see usage of a LoadBalanced RestTemplate. It’s a first step for using Hystrix and @EnableCircuitBreaker. Isn’t it?

    • @LoadBalanced is used to enable client side load balancing. For example, if more than one Customers Service is available, subsequent requests could be dispatched to different instances according to balancing strategy (round robin is the default in Ribbon).
  • You also introduce usage of named service (ie. http://customers-service/owners/{ownerId}). It’s great.

    • It is possible thanks to Ribbon and @LoadBalanced RestTemplate as described in this tutorial.
  • I am not familial with Lombok and syntax like this: @requiredargsconstructor(onConstructor = @__(@Autowired)). Do you really want to use Lombok?

    • Personally I'm not a big fan of Lombok, but on the other hand it can help us to limit a boilerplate code and focus on Spring Boot/Cloud related stuff. I see Lombok is widely used in other Spring sample projects.
  • Classes VisitDetails, PetDetails, OwnerDetails and PetType has been added. They duplicates Visit, Pet, Owner and PetType model classes. I know it’s a bad practice to share thoses classes between micro-services. But in the Api Gateway?

    • Those classes are not a duplication, but a "view" of a model from Api Gateway perspective. As long as Api can access data described by added classes, interface of particular service can change. This relation can be better maintained using Consumer Driven Contracts.
  • For each Pet, VisitsServiceClient does a REST call. ParallelStream is used. But for better performance, the visits-service may provides an API with takes a list of pets id. No ?

    • You're right, I even added a TODO for that improvement.

Maybe @dsyer can help us to deal with our doubts?

@arey
Copy link
Member

arey commented Nov 28, 2016

Hi @mszarlinski, just a few commnts to some answers:

  • The Api Gateway is not just an API. It now does data aggregation. We also could fetch visits from the Angular client side. What is the best in a micro services architecture?

    • The question is what is Api Gateway responsible for - it is only a router or provides some services too. I must admit my implementation is based on Springbox sample application. Of course, we could do data aggregation on a client, but I think we should strive to hide internal microservices structure which can change over time whereas API remains stable. More on that topic is written here. I think it is a good topic to discuss.
    • In the API gateway pattern article, we also could read: *the API gateway enables clients to retrieve data from multiple services with a single round-trip. Fewer requests also means less overhead and improves the user experience. An API gateway is essential for mobile applications *. Thus an API gateway is not a simpler router. From the author's point of view, it could call multiple routers for data aggregation. We could go on.
  • You also introduce usage of named service (ie. http://customers-service/owners/{ownerId}). It’s great.

    • It is possible thanks to Ribbon and @LoadBalanced RestTemplate as described in this tutorial.
    • To active Ribbon, do we need to use the @RibbonClient on all rest clients (i.e. CustomersServiceClient)?
  • I am not familial with Lombok and syntax like this: @requiredargsconstructor(onConstructor = @__(@Autowired)). Do you really want to use Lombok?

    • Personally I'm not a big fan of Lombok, but on the other hand it can help us to limit a boilerplate code and focus on Spring Boot/Cloud related stuff. I see Lombok is widely used in other Spring sample projects.
    • Ok. I will learn.
  • Maybe @dsyer can help us to deal with our doubts?

    • Yes, Dave Syer is the founder of Spring Cloud. He should be able to deal with our doubts. Maybe also @ryanjbaxter. He wrote a lof of articles about Spring Cloud. I don't know if they are notified like in Twitter when you reference their username with an @ in github.

@mszarlinski
Copy link
Collaborator Author

  • You also introduce usage of named service (ie. http://customers-service/owners/{ownerId}). It’s great.

    • It is possible thanks to Ribbon and @LoadBalanced RestTemplate as described in this tutorial.
    • To active Ribbon, do we need to use the @RibbonClient on all rest clients (i.e. CustomersServiceClient)?
    • @RibbonClient annotation is used to customize Ribbon defaults, I think we can proceed with default settings.
  • Maybe @dsyer can help us to deal with our doubts?

    • Yes, Dave Syer is the founder of Spring Cloud. He should be able to deal with our doubts. Maybe also @ryanjbaxter. He wrote a lof of articles about Spring Cloud. I don't know if they are notified like in Twitter when you reference their username with an @ in github.
    • GitHub @mentions should work. Maybe let's wait until end of week for their review. I would rebase and merge it on Monday, 5th of Dec.

@mszarlinski mszarlinski force-pushed the master branch 2 times, most recently from dbe207a to 5cccfc9 Compare December 4, 2016 19:13
@mszarlinski mszarlinski merged commit ae7a717 into spring-petclinic:master Dec 5, 2016
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.

2 participants