Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: restructure core by component #1842

Merged
merged 5 commits into from
Aug 22, 2022
Merged

feat: restructure core by component #1842

merged 5 commits into from
Aug 22, 2022

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Aug 17, 2022

What this PR changes/adds

move all the core folders into the core folder, grouped by the component they are needed by:

core
├── common
│   ├── base
│   ├── boot
│   ├── policy-engine
│   └── policy-evaluator
├── control-plane
│   ├── contract
│   ├── control-plane-core
│   └── transfer
├── data-plane
│   ├── data-plane-core
│   └── data-plane-framework
├── data-plane-selector
│   └── data-plane-selector-core
└── federated-catalog
    ├── federated-catalog-cache
    └── federated-catalog-core

Why it does that

To accomplish project structure review

Further notes

  • All the chickens are coming home to roost 🐔 🐓 🐔
  • As already reported in feat: restructure spi by component #1832: we need to untangle the policy knots, let's talk about this in the discussion: Policy classes and interfaces structure #1836
  • I'm not really sure why boot and base are separated, they could be the same module, and they are used pretty much always together (unifying them plus refactoring the policy ones could avoid that ugly common folder)
  • inlined selector-store into selector-core, as it was only a default provider for the DefaultDataPlaneInstanceStore
  • inlined catalog and substituted by federated-catalog-core
  • introduced control-plane-core and inlined defaults, as it was pretty straightforward. Seems like the core module of a specific component is the right place where to declare default components to-be registered by extensions
  • I have a doubt about selector-api and selector-client, they are extensions used to build a data-plane-selector, but at the same time could a data-plane-selector exist without them? if not, I would put them as core modules for data-plane-selector, but they are completely fine as exception as they are.

Linked Issue(s)

Closes #1809

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • assigned appropriate label? (exclude from changelog with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@ndr-brt ndr-brt added the refactoring Cleaning up code and dependencies label Aug 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2022

Codecov Report

Merging #1842 (8042c90) into main (ef3a917) will decrease coverage by 0.02%.
The diff coverage is 5.00%.

@@            Coverage Diff             @@
##             main    #1842      +/-   ##
==========================================
- Coverage   62.52%   62.50%   -0.03%     
==========================================
  Files         780      781       +1     
  Lines       16635    16638       +3     
  Branches     1085     1085              
==========================================
- Hits        10401    10399       -2     
- Misses       5784     5789       +5     
  Partials      450      450              
Impacted Files Coverage Δ
...dataspaceconnector/core/CoreServicesExtension.java 85.00% <ø> (ø)
...onnector/core/base/CommandHandlerRegistryImpl.java 50.00% <ø> (ø)
...aspaceconnector/core/base/OkHttpClientFactory.java 92.85% <ø> (ø)
...core/base/RemoteMessageDispatcherRegistryImpl.java 14.28% <ø> (ø)
...r/core/base/agent/ParticipantAgentServiceImpl.java 100.00% <ø> (ø)
...econnector/core/base/policy/PolicyContextImpl.java 38.88% <ø> (ø)
...ceconnector/core/base/policy/PolicyEngineImpl.java 82.81% <ø> (ø)
...ctor/core/base/policy/RuleBindingRegistryImpl.java 100.00% <ø> (ø)
...taspaceconnector/core/base/policy/ScopeFilter.java 92.64% <ø> (ø)
...nector/core/defaults/DefaultServicesExtension.java 83.33% <ø> (ø)
... and 135 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly addressing your "further notes"

  • All the chickens are coming home to roost 🐔 🐓 🐔

    best comment so far.

  • boot vs base: they're separate because they serve distinct purposes: boot takes care of runtime boostrap and extension loading, whereas base provides some basic (but optional) features. Some runtimes will only require the boot module for bootstrapping, but will not need the base module. Thus having them separate still makes sense to me.

  • selector-store: as per the (updated) documentation, a default provider for a service should be in its own extension, even if it does not use any other injected fields.

  • control-plane-core: I would rename the ControlPlaneCoreExtension to something like DefaultStoresExtension or something to clearly state what it's doing.

  • selector-api and selector-client: architecturally, a data-plane-selector should not need them. For example when running the selector embedded. They are only needed when the selector runs in its own process (e.g. as pod in a K8s). If there is currenlty a dependency it should be cleaned up (in another PR, I can help out in that regard)

  • federated-catalog-core is empty, except for a build file. on purpose?

  • minor nit: in some files the Copyright seems to have been captured by BMW :) e.g. ControlPlaneCoreExtension - the original Copyright is by Microsoft. could have happened in others as well.

@ndr-brt
Copy link
Member Author

ndr-brt commented Aug 19, 2022

  • boot vs base: they're separate because they serve distinct purposes: boot takes care of runtime boostrap and extension loading, whereas base provides some basic (but optional) features. Some runtimes will only require the boot module for bootstrapping, but will not need the base module. Thus having them separate still makes sense to me.

ok

  • selector-store: as per the (updated) documentation, a default provider for a service should be in its own extension, even if it does not use any other injected fields.
  • control-plane-core: I would rename the ControlPlaneCoreExtension to something like DefaultStoresExtension or something to clearly state what it's doing.
  • federated-catalog-core is empty, except for a build file. on purpose?

These three points are strictly related, in my opinion every component should have a "core" extension that defines the base services and the default ones, for the control plane is ControlPlaneCoreExtension, and for the data-plane-selector is DataPlaneSelectorExtension.
data-plane and federated-catalog have no default services, that's why their -core module is just a BOM build file.

Having a -core module that gathers the base needed dependencies and that provides the default services will permit extenders to just add that dependency on their build file and then extensions on top of it (this idea could be part of #1596).

Seems a clean pattern to me.

EDIT: I also created and forgot at issue for this 🥲 #1812

  • selector-api and selector-client: architecturally, a data-plane-selector should not need them. For example when running the selector embedded. They are only needed when the selector runs in its own process (e.g. as pod in a K8s). If there is currenlty a dependency it should be cleaned up (in another PR, I can help out in that regard)

Ok

  • minor nit: in some files the Copyright seems to have been captured by BMW :) e.g. ControlPlaneCoreExtension - the original Copyright is by Microsoft. could have happened in others as well.

Fixed

@paullatzelsperger
Copy link
Member

I like the -core module pattern, that is fine.
However:

  • selector-store: as per the (updated) documentation, a default provider for a service should be in its own extension, even if it does not use any other injected fields.

The problem there is that it is the same extension (DataPlaneSelectorExtension) that does the @Inject-ing and @Provider-ing of the same service (i.e. DataPlaneInstanceStore), which - according to the documentation - should be avoided. For now it would be sufficient to just have another extension in the data-plane-selector-core module, that only contains the provider method. As a future enhancement, that should to be cleaned up further, e.g. by renaming the store to InMemory-, and providing it from another module etc.

  • federated-catalog-core is empty, except for a build file. on purpose?

These three points are strictly related, in my opinion every component should have a "core" extension that defines the base services and the default ones, for the control plane is ControlPlaneCoreExtension, and for the data-plane-selector is DataPlaneSelectorExtension. data-plane and federated-catalog have no default services, that's why their -core module is just a BOM build file.

OK that's fine, but now we have

federated-catalog-cache <-- contains code
federated-catalog-core <-- contains only the bom

could we just merge/move everything from -cache over to -core?

@ndr-brt ndr-brt merged commit efe29b3 into eclipse-edc:main Aug 22, 2022
@ndr-brt ndr-brt deleted the feature/1809-restructure-core-modules branch August 22, 2022 06:53
janpmeyer pushed a commit to FraunhoferISST/edc-connector that referenced this pull request Aug 23, 2022
* Move contract and transfer core modules into control-plane

* PR remarks

* Fix CI

* Extract instance store provision to a specific extension

* Rename federated-catalog-cache to federated-catalog-core
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Cleaning up code and dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project structure: core modules
3 participants