Skip to content

Instantly share code, notes, and snippets.

@tjquinno
Last active February 17, 2021 13:53
Show Gist options
  • Save tjquinno/179c130519cbcbebb5009823bc83d74c to your computer and use it in GitHub Desktop.
Save tjquinno/179c130519cbcbebb5009823bc83d74c to your computer and use it in GitHub Desktop.
Discussion of module interdependencies and class inheritance highlighted by Micrometer support
I did not want to continue the (bad) practice of copying code from existing `xxxSupport` and
`xxxCdiExtension` classes into the new Micrometer classes. The Micrometer classes share
considerable behavior with the Helidon metrics ones. You might remember that my first PR
added the Micrometer classes to the existing metrics modules, and you (correctly) pointed
out that would drag along unneeded dependencies.
When I then created the separate `integrations/micrometer` module, I rejected putting the common
classes into either it or `microprofile/metrics` and have one depend on the other; that, too,
would drag along unused dependencies.
Instead, I created the two new `common` modules, not knowing that you want modules under `common`
to be SE only.
In your PR comments you raised a good point that not all (although I think quite a few) extensions
need the server dependency and the endpoint functionality.
With that in mind, the full solution could eventually look something like this:
MetricsSupport --------+
+-- extends --> RestServiceSupportBase -- extends --> ServiceSupportBase
MicrometerSupport -----+
and
MetricsCdiExtension ----+
+-- extends --> RestCdiExtensionBase -- extends --> CdiExtensionBase
MicrometerCdiExtension -+
This would accurately reflect our hierachy. Non-REST support classes and extensions would subclass the
right-most base classes.
The Micrometer PR would exclude CdiExtensionBase and ServiceSupportBase. (Later on, if it made sense,
we could add those right-most classes and migrate non-REST `xxxSupport` classes and extensions so they
would extend those classes.)
If you are OK with that class hierarchy plan for this PR, the question becomes which modules should
hold the `RestXXXBase` classes and associated types?
It seems you are OK with a module in `common` that would depend on the SE server and would hold
`RestServiceSupportBase` since it would be SE-only. Maybe `common/rest-service-support`
For CDI you suggested possibly creating a new `microprofile/common` module to hold what I've now called
`RestCdiExtensionBase`. Remember that non-MP extensions (like Micrometer) would subclass from
that so it would not truly be limited to MicroProfile technologies. And it would depend on the MP server.
(I'm not sure I see the advantage in limiting all submodules under `common` to SE only, and having it
there would make it clear we don't think it's MP only. But not a big deal.)
Are you still OK with the module name `microprofile/common` and the dependencies I've outlined?
If not, other suggestions?
@tjquinno
Copy link
Author

Thanks for the notes.

In the structure you proposed, splitting apart the lower-level common and rest submodules does seem to make sense.

So the proposed new modules would be:

common/
  service-support/
    common/ - common types for xxxSupport in SE without REST (maybe not needed until later)
    rest/ - common types for xxxSupport in SE with REST
microprofile/
  common/
    common/ - common types for xxxCdiExtension in  MP without REST
    rest/ - common types for xxxCdiExtension in MP with REST

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment