feat: implement policy-monitor #3456
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndr-brt
requested review from
paullatzelsperger,
juliapampus and
jimmarino
as code owners
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3456 +/- ##
==========================================
- Coverage 72.11% 71.70% -0.41%
==========================================
Files 836 841 +5
Lines 16989 17002 +13
Branches 947 950 +3
==========================================
- Hits 12252 12192 -60
- Misses 4333 4407 +74
+ Partials 404 403 -1
☔ View full report in Codecov by Sentry. |
paullatzelsperger
approved these changes
Sep 15, 2023
| .additional(ContractAgreement.class, contractAgreement) | ||
| .build(); | ||
|
|
||
| var result = policyEngine.evaluate("transfer.process", policy, policyContext); |
There was a problem hiding this comment.
we already have that defined as constant on the ContractValidationService. Either we re-use that (possibly moving to a more common class), or we re-declare a constant here and annotate with @PolicyScope
There was a problem hiding this comment.
will note this on my todo for the next PR 👍
...src/main/java/org/eclipse/edc/connector/policy/monitor/manager/PolicyMonitorManagerImpl.java
Show resolved
Hide resolved
wolf4ood
approved these changes
Sep 15, 2023
jimmarino
requested changes
Sep 18, 2023
...src/main/java/org/eclipse/edc/connector/policy/monitor/manager/PolicyMonitorManagerImpl.java
Show resolved
Hide resolved
...src/main/java/org/eclipse/edc/connector/policy/monitor/store/InMemoryPolicyMonitorStore.java
Show resolved
Hide resolved
...src/main/java/org/eclipse/edc/connector/policy/monitor/manager/PolicyMonitorManagerImpl.java
Show resolved
Hide resolved
...-core/src/main/java/org/eclipse/edc/connector/policy/monitor/subscriber/StartMonitoring.java
Show resolved
Hide resolved
...spi/src/main/java/org/eclipse/edc/connector/policy/monitor/spi/PolicyMonitorEntryStates.java
Show resolved
Hide resolved
...nitor-spi/src/main/java/org/eclipse/edc/connector/policy/monitor/spi/PolicyMonitorEntry.java
Show resolved
Hide resolved
...tor-spi/src/main/java/org/eclipse/edc/connector/policy/monitor/spi/PolicyMonitorManager.java
Outdated
Show resolved
Hide resolved
...tor-core/src/main/java/org/eclipse/edc/connector/core/entity/AbstractStateEntityManager.java
Show resolved
Hide resolved
...tor-core/src/main/java/org/eclipse/edc/connector/core/entity/AbstractStateEntityManager.java
Outdated
Show resolved
Hide resolved
spi/common/core-spi/src/main/java/org/eclipse/edc/spi/entity/StateEntityManager.java
Outdated
Show resolved
Hide resolved
jimmarino
requested changes
Sep 18, 2023
bscholtes1A
reviewed
Sep 20, 2023
|
|
||
| import org.eclipse.edc.spi.persistence.StateEntityStore; | ||
|
|
||
| public interface PolicyMonitorStore extends StateEntityStore<PolicyMonitorEntry> { |
There was a problem hiding this comment.
Do we need a dedicated store for that?
Can't we just browse the active ContractAgreement (i.e. not expired nor canceled) stored in the contract negotiation store?
There was a problem hiding this comment.
we need the state machine for reliability, scalability and performances (the reason was detailed in the related DR)
furthermore, ContractAgreements don't have an "expired" or "canceled" state, is a condition that needs to be verified on the policy, so otherwise we'd need to query every ContractAgreement every time as there's no way to mark them "expired" nor "canceled".
ndr-brt
force-pushed
the
3445-policy-monitor-embedded
branch
from
1479547
to
1a20f85
Compare
jimmarino
approved these changes
Sep 26, 2023
ndr-brt
force-pushed
the
3445-policy-monitor-embedded
branch
from
1a20f85
to
e4ca9c7
Compare
bscholtes1A
approved these changes
Sep 26, 2023
ndr-brt
force-pushed
the
3445-policy-monitor-embedded
branch
from
e4ca9c7
to
065bcab
Compare
ndr-brt
force-pushed
the
3445-policy-monitor-embedded
branch
from
065bcab
to
b87ff07
Compare
ndkrimbacher
pushed a commit
to nexyo-io/DataSpaceConnector
that referenced
this pull request
Oct 4, 2023
* feat: implement policy-monitor * Pull store to AbstractStateEntityManager * PR remarks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR changes/adds
Implements the first version of the
PolicyMonitor: embedded in the controlplane, with in-memory storageWhy it does that
streaming data transfer
Further notes
StateManagerhierarchy, that permits to avoid some duplication, this can be pushed a little further, will do it in the subsequent PRsEndToEndKafkaTransferTestis showing the PolicyMonitor in action, defining a policy that expires in 10s, the transfer gets completed. there still is a missing part to actually shut down the data flow, described in this issue: policy-monitor: data flow should be stopped on transfer completion #3453InMemoryPolicyMonitorStoreis not tested as it is a simple wrapper of theInMemoryStatefulEntityStore, tests will be added with the implementation of the Sql storage (policy-monitor: sql storage #3447 )Linked Issue(s)
Closes #3445
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.