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: add federated catalog samples for an embedded and a standalone federated catalog #342

Merged

Conversation

farhin23
Copy link
Contributor

What this PR changes/adds

This PR adds a new module federated-catalog, which includes the following samples,

  • fc-00-basic: includes preparations for the later Federated Catalog samples
  • fc-01-embedded: demonstration of a federated catalog, embedded in a connector.
  • fc-02-standalone: demonstrates the implementation of a standalone catalog.

Why it does that

To illustrate how we can implement different types of Federated Catalogs, e.g embedded and standalone.

Further notes

Projects and libraries have been added in settings.gradle.kts, and gradle/libs.versions.toml.

Linked Issue(s)

Contributes to #296

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@farhin23 farhin23 added the enhancement New feature or request label Nov 19, 2024
@farhin23 farhin23 mentioned this pull request Nov 19, 2024
3 tasks
@ndr-brt ndr-brt self-requested a review November 20, 2024 14:39
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

please provide automated tests on the system-tests module that ensure the samples are working correctly, as done for the other samples

@farhin23 farhin23 force-pushed the feat/federated-catalog-sample branch from 6897586 to 246b424 Compare November 29, 2024 11:23
@farhin23 farhin23 requested a review from ndr-brt November 29, 2024 17:07
@farhin23
Copy link
Contributor Author

please provide automated tests on the system-tests module that ensure the samples are working correctly, as done for the other samples

Automated tests have been added for the samples.

@farhin23 farhin23 requested a review from ndr-brt December 5, 2024 13:32
federated-catalog/README.md Outdated Show resolved Hide resolved
@farhin23
Copy link
Contributor Author

farhin23 commented Dec 5, 2024

The pipeline tests are experiencing unpredictable failures. In the latest commit, one of the tests in Policy01Basic fails despite no changes being made in this area.

Probable cause:
Since this is a time-dependent test, a likely cause could be the repetitive tasks in FederatedCatalog runtimes that continue running even after the associated runtimes are shut down.
Although the shutdown methods for RuntimeExtension terminate the runtimes, they do not stop the ScheduledExecutorService responsible for recursive crawling within FederatedCatalog, which remains active and continues to take up resources. This might be causing delays in the execution of the Policy01Basic test, ultimately leading to its failure.

Solution:
Proper shutdown method in FederatedCatalogCacheExtension to trigger the shutdown of ScheduledExecutorService should solve this issue. This will ensure that all the scheduled tasks are terminated properly at the time of runtime shutdown.
Regardless of the current pipeline failure, the scheduled tasks in FederatedCatalog requires a proper shutdown service.

@ndr-brt, @paullatzelsperger could you please confirm if I have understood this correctly or missed something? If this is indeed a valid issue, then I would like to open an issue on this.

@ndr-brt
Copy link
Member

ndr-brt commented Dec 5, 2024

The pipeline tests are experiencing unpredictable failures. In the latest commit, one of the tests in Policy01Basic fails despite no changes being made in this area.

Probable cause: Since this is a time-dependent test, a likely cause could be the repetitive tasks in FederatedCatalog runtimes that continue running even after the associated runtimes are shut down. Although the shutdown methods for RuntimeExtension terminate the runtimes, they do not stop the ScheduledExecutorService responsible for recursive crawling within FederatedCatalog, which remains active and continues to take up resources. This might be causing delays in the execution of the Policy01Basic test, ultimately leading to its failure.

this is definitely an issue that needs to be fixed in the FederatedCatalog repo, would you mind to open an issue? (end eventually provide a PR to fix it?)

Solution: Proper shutdown method in FederatedCatalogCacheExtension to trigger the shutdown of ScheduledExecutorService should solve this issue. This will ensure that all the scheduled tasks are terminated properly at the time of runtime shutdown. Regardless of the current pipeline failure, the scheduled tasks in FederatedCatalog requires a proper shutdown service.

that's correct, but we need a workaround right here to make it pass, like increasing the timeout or increasing the "period seconds" setting in the FC runtime configuration

Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

anyway, it just passed, so LGTM and please open an issue in the FCC repository about the thread shutdown. I guess tests will remain flake until the fix will be rolled out in a new version. Given that this is the Samples repos I guess we can bear with that.

@farhin23
Copy link
Contributor Author

farhin23 commented Dec 5, 2024

this is definitely an issue that needs to be fixed in the FederatedCatalog repo, would you mind to open an issue? (end eventually provide a PR to fix it?)

I will open an issue and take care of the fix.

that's correct, but we need a workaround right here to make it pass, like increasing the timeout or increasing the "period seconds" setting in the FC runtime configuration

I will leave it for now, as the current commit just passed. And will make necessary adjustments in future commits.

@ndr-brt ndr-brt merged commit ab681f6 into eclipse-edc:main Dec 6, 2024
6 checks passed
@ronjaquensel ronjaquensel deleted the feat/federated-catalog-sample branch December 10, 2024 07:28
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.

2 participants