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?
@tomas-langer
Copy link

The reason for limiting submodules under root modules (except for integration, microprofile and examples to SE only:
Microprofile was built on top of Helidon SE, so all code related to it was in a separate directory (and package structure), to make this distinction clear.
I do realize this may be outdated now, as we have decided to move parts of Helidon together in a single module (supporting both SE and MP).

I think the important question now would be - "Do we want to merge together core modules of Helidon?" - such as metrics, tracing...
If the answer is "YES, we want to merge them for 3.x" - then we can start building a common/mp module to be used by these modules.
If the answer is "NO, only integrations may be MP/SE" - then we should have an microprofile/common module.

I think this deserves a POC to see how it changes size of Helidon, and if it is feasible at all. Until then I would prefer to have microprofile/common

Regarding the hierarchy - in SE, there is no "server without REST" - so there is no need to create a non-rest service support.

Suggestion for MP:

  • CdiExtensionBase (maybe HelidonCdiExtension) - in module microprofile/common/common
  • RestCdiExtensionBase (maybe HelidonRestCdiExtension) - in module microprofile/common/rest

Please do not change it yet, let's first make sure this is OK for us (and others).

@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