cashapp / misk

Microservice Kontainer
https://cashapp.github.io/misk/
Apache License 2.0
397 stars 169 forks source link

Break up misk module #1330

Open jjestrel opened 4 years ago

jjestrel commented 4 years ago

See also #1044 The misk module currently contains several groups of things:

We should separate the misk module into something like these four pieces: misk-core misk-server misk-client misk-actions

Additionally we should merge misk-inject into misk-core

ryanhall07 commented 4 years ago

Additionally we should merge misk-inject into misk-core

what is the benefit of doing this? just less modules and there isn't a high probability you just want our guice code?

misk-actions

why can't this be in misk-server? tests probably need the misk-server stuff if they want to run a real app

this sounds reasonable to me. It would be nice to give guidance about the modules, especially something like misk-core. is that just everything that is shared between misk-client and misk-server?

jjestrel commented 4 years ago

Additionally we should merge misk-inject into misk-core

what is the benefit of doing this? just less modules and there isn't a high probability you just want our guice code?

misk-inject is two files in a module that have a dependency on guice and kotlin. All our other modules will have the same dependencies on guice and kotlin. It seems worth rolling it in to something common (misk-core) if we promise to keep the dependencies to guice, kotlin, and logging

misk-actions

why can't this be in misk-server? tests probably need the misk-server stuff if they want to run a real app

I think there may be benefits if we structure the app as actions being built on top of something misk-server provides

this sounds reasonable to me. It would be nice to give guidance about the modules, especially something like misk-core. is that just everything that is shared between misk-client and misk-server?

I think that's exactly the distinction. The two entrypoints of misk are either including misk-client and building a client, or misk-server and building a server. misk-core is everything shared by both

zhxnlai commented 4 years ago

Additionally we should merge misk-inject into misk-core

I actually wrote a library whose public api contains a subclass of KAbstractModule. Because this library exposes KAbstractModule, I made misk-inject its api dependency so that this library is easier to consume. I think we should keep misk-inject a standalone module for this purpose.

swankjesse commented 4 years ago

@acostama and I pulled out the misk-actions package today. We also wrote some early documentation on the boundary of each module. So far we’ve only documented the modules that have meaningful boundaries!

swankjesse commented 4 years ago

We’ve made good progress on this but I think there’s further steps to take. Here’s what I propose next, much of which is natural next steps from @jjestrel and @ryanhall07’s discussion above.

I don’t like either the misk-core or misk modules because they lack definition. I’d prefer for us to say what API and implementation a module offers rather than what dependencies it has.

ryanhall07 commented 4 years ago

we should also break up the misk-aws module. e.g the sqs job queue impl should not be in there just because it uses sqs. misk-aws should really just contain the code to establish an aws session with credentials.