misk icon indicating copy to clipboard operation
misk copied to clipboard

Break up misk module

Open jjestrel opened this issue 5 years ago • 6 comments

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

  • Relatively dependency free things: clustering, config, environment, metrics(?), token gen, etc
  • Clients: misk.client, including TypedHttpClient, etc
  • Server: misk.web.jetty, including some other helpers
  • Actions: most of the existing misk module code; anything that touches WebAction. clients also depend on this in tests today.

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

jjestrel avatar Dec 16 '19 21:12 jjestrel

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?

ryanhall07 avatar Dec 16 '19 22:12 ryanhall07

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

jjestrel avatar Dec 17 '19 17:12 jjestrel

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.

zhxnlai avatar Dec 17 '19 19:12 zhxnlai

@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 avatar May 04 '20 15:05 swankjesse

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.

  • [ ] misk-observability: logging, metrics, and tracing. These things are entangled anyway. We should have separate interface and implementation modules. There’s so much stuff here: jaeger, zipkin, prometheus, metrics-digester, datadog. Is it all useful?
  • [ ] misk-clustering: plus implementations misk-clustering-k8s, misk-clustering-zookeeper. Do we need both implementations?
  • [ ] misk-concurrent: clocks, sleepers, and executor services.
  • [ ] misk-http-client: Retrofit, gRPC, and OkHttp glue
  • [ ] misk-http-server: Jetty glue, including all of the JSON, protobuf, and gRPC marshalling

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.

swankjesse avatar May 04 '20 15:05 swankjesse

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.

ryanhall07 avatar May 04 '20 16:05 ryanhall07