Thanks for writing a very detailed design doc. My feedback is below: - The title of the SEP is Container Placements in Samza, which sounds like it's going to reimplement the container placements logic currently in ClusterBasedJobCoordinator. After reading the proposal, I think it should be renamed to be a much smaller scope, e.g. Container Relocation Tool. - Goal: the goal is to build a tool, right? I want to be crystal clear on that. After the solution, I think there will be an tool to do container relocation for Samza open source. Build the ability doesn't mean much to Samza open source users I believe. - Proposed Solution: after reading this I think the rejected the option is to write to metadastore, and the accepted one is to develop a container placement service. But then I read the architecture part, the proposal says the preferred approach is to use Samza metastore API to read and write the container placement metadata. It feels contradictory to me. Seems the metadatsstore is a must-have piece in this solution, so it might be cleaner to remove the first solution. - I don't understand the "service" part of the container placement. Is it a separate service that can be hosted somewhere? Is it based on jetty or netty? what's the communication protocol? From reading the proposal, it looks like a set of API instead of service. If that's the case, please remove all the usage of "container placement service" in this proposal. - key-value format: the description of the uuid of the ContainerPlacementRequestMessage has a typo I think (response -> request). - The diagram of the components interaction look nice, and definitely helps me understand what the solution will look like. I didn't find a description about writing the new locality information. I am interested in the ordering of that regarding to the ordering of reserving resource and running the container. I feel that part will be a bit complicated. - public interfaces: are these interfaces intended to by used by samza users? How do they use them? Is there an example? Are these interfaces going into samza-api? Are those interfaces stable? Why the uuid in the interface cannot be generated by Samza itself? Why ContainerPlacementMetadatstore is part of the API? should that be hidden from the users? If the goal is to provide a tool, I would rather make these classes internal in samza-core and also describe how the tool will look like here.
Thanks, Xinyu On Wed, Jan 15, 2020 at 10:34 AM Sanil Jain <sanil.jai...@gmail.com> wrote: > Hi Prateek, > > Thanks for the feedback, I have updated the SEP with your suggestions. > Please have a look. > > Thank you > Sanil > > On Mon, 6 Jan 2020 at 10:54, Sanil Jain <sanil.jai...@gmail.com> wrote: > > > Hi all, > > > > Refreshing this back to see if there is any further feedback on this SEP. > > > > Thanks > > Sanil > > > > On Thu, 5 Dec 2019 at 16:25, Sanil Jain <sanil.jai...@gmail.com> wrote: > > > >> Hi all, > >> > >> I have created SEP-22: Container Placements in Samza, which adds > >> abilities to move containers for a Samza job seamlessly from one host to > >> another without restarting jobs. > >> > >> Please find the SEP wiki below: > >> > https://cwiki.apache.org/confluence/display/SAMZA/SEP-22%3A+Container+Placements+in+Samza > >> > >> Please take a look and chime in your feedback. > >> > >> Thanks > >> Sanil > >> > > >
