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(catalog): simplify the FederatedCatalog #1741

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented Jul 25, 2022

What this PR changes/adds

Cleans out a bit of unnecessary code of the FederatedCatalog. In particular the following classes were removed:

  • PartitionManager + implementors
  • LoaderManager + implementors
  • WorkItemQueue + implementors
  • Loader + implementors

Instead, the entire crawler subsystem has been simplified to:

  • Extension creates ExecutionManager
  • ExecutionManager runs ExecutionPlan:
    • instantiates N crawlers (N ... configuration value)
    • get items from FederatedCacheNodeDirectory (these are the crawl targets)
    • while there are still items, take a crawler and set it off
    • has a pre and a post hook

Why it does that

Simplicity is good for readability and maintainability. And mental sanity.

Further notes

.

Linked Issue(s)

Closes #1733

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)

@paullatzelsperger paullatzelsperger added the enhancement New feature or request label Jul 25, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1741 (b193394) into main (8c924fd) will decrease coverage by 0.02%.
The diff coverage is 87.82%.

@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
- Coverage   67.34%   67.31%   -0.03%     
==========================================
  Files         785      781       -4     
  Lines       16862    16758     -104     
  Branches     1062     1057       -5     
==========================================
- Hits        11356    11281      -75     
+ Misses       5050     5020      -30     
- Partials      456      457       +1     
Impacted Files Coverage Δ
...aceconnector/catalog/spi/model/UpdateResponse.java 0.00% <ø> (ø)
.../catalog/cache/FederatedCatalogCacheExtension.java 76.74% <73.07%> (-3.59%) ⬇️
...spaceconnector/catalog/cache/ExecutionManager.java 88.65% <88.65%> (ø)
...onnector/catalog/cache/crawler/CatalogCrawler.java 95.45% <95.45%> (ø)
...tor/catalog/cache/query/BatchedRequestFetcher.java 88.88% <100.00%> (+1.38%) ⬆️
...alog/cache/query/IdsMultipartNodeQueryAdapter.java 26.66% <100.00%> (ø)
...spaceconnector/catalog/spi/CacheConfiguration.java 68.42% <100.00%> (ø)
...og/cache/crawler/NodeQueryAdapterRegistryImpl.java 40.00% <0.00%> (-20.00%) ⬇️
...iation/ProviderContractNegotiationManagerImpl.java 90.00% <0.00%> (-0.56%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c924fd...b193394. Read the comment docs.

@paullatzelsperger paullatzelsperger force-pushed the feature/1733_simplify_federatedcatalog branch from 5293e36 to ff5578c Compare July 26, 2022 13:12
@paullatzelsperger paullatzelsperger marked this pull request as ready for review July 26, 2022 13:12
Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

✂️

@paullatzelsperger paullatzelsperger merged commit 3ef48d5 into eclipse-edc:main Jul 26, 2022
diegogomez-zf pushed a commit to diegogomez-zf/DataSpaceConnector that referenced this pull request Aug 3, 2022
* removed partition manager, workitem queue and old crawler

* introduced execution manager

* cleanup

* added crawler test

* added test for ExecutionManager

* moved back to using explicit mocking

* moved the successhandler back into the crawler

* use test utils for port

* feat: add tests for FC extension

* source doc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify FederatedCatalog
5 participants