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

fix: compilation #128

Merged
merged 1 commit into from
Jul 31, 2023
Merged

fix: compilation #128

merged 1 commit into from
Jul 31, 2023

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Jul 31, 2023

What this PR changes/adds

Fix compilation that was broken because of upstream changes

Why it does that

Briefly state why the change was necessary.

Further notes

List other areas of code that have changed but are not necessarily linked to the main feature. This could be method signature changes, package declarations, bugs that were encountered and were fixed inline, etc.

Linked Issue(s)

Closes # <-- insert Issue number if one exists

@ndr-brt ndr-brt added bug Something isn't working build labels Jul 31, 2023
@github-actions
Copy link

github-actions bot commented Jul 31, 2023

Test Results

48 tests  ±0   48 ✔️ ±0   53s ⏱️ +6s
11 suites ±0     0 💤 ±0 
11 files   ±0     0 ±0 

Results for commit 8cbc1b1. ± Comparison against base commit 4ba9dc9.

♻️ This comment has been updated with latest results.

@ndr-brt ndr-brt changed the title fix compilation fix: compilation Jul 31, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: -0.16% ⚠️

Comparison is base (4ba9dc9) 65.58% compared to head (8cbc1b1) 65.43%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   65.58%   65.43%   -0.16%     
==========================================
  Files          25       25              
  Lines         433      434       +1     
  Branches       15       15              
==========================================
  Hits          284      284              
- Misses        134      135       +1     
  Partials       15       15              
Files Changed Coverage Δ
...ache/FederatedCatalogDefaultServicesExtension.java 0.00% <0.00%> (ø)
...edc/catalog/store/InMemoryFederatedCacheStore.java 96.15% <66.66%> (-3.85%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

private final LockManager lockManager;

public InMemoryFederatedCacheStore(CriterionConverter<Predicate<Catalog>> converter, LockManager lockManager) {
this.converter = converter;
public InMemoryFederatedCacheStore(LockManager lockManager) {
Copy link
Member

@paullatzelsperger paullatzelsperger Jul 31, 2023

Choose a reason for hiding this comment

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

Having the converter "configurable" through constructor injection is valuable here, because it's very likely that in the future, we'll need an "actual" predicate converter. Even if currently, we only use the alwaysTrue one.

For context: here the in-mem implementation is not just used for testing, but very likely will be used in production deployments as well, because the cache store is ephemeral in nature. At least a PG (or even Cosmos) one was not done in the past for that exact reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the change will happen in the store itself, I mean, it's part of the in-memory implementation behavior, I don't see any value in having it injectable (with all the implementations using the same instance), in the connector stores this converter is instantiated by the stores internally.

@paullatzelsperger paullatzelsperger merged commit ac96fdd into main Jul 31, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants