-
Notifications
You must be signed in to change notification settings - Fork 245
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: dataplane self-registration #4166
feat: dataplane self-registration #4166
Conversation
6e84999
to
dd2888a
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #4166 +/- ##
==========================================
+ Coverage 71.74% 75.04% +3.29%
==========================================
Files 919 1012 +93
Lines 18457 20547 +2090
Branches 1037 1160 +123
==========================================
+ Hits 13242 15419 +2177
+ Misses 4756 4620 -136
- Partials 459 508 +49 ☔ View full report in Codecov by Sentry. |
dd2888a
to
1de1459
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the turnCount
was originally intended to track how often each data plane was selected to serve a transfer, to implement a round-robin-like selector strategy.
Together with the turnCount
I would put the entire selection strategy idea up for discussion - it seems dataplanes are selected by capability much more than by some high-level load-balancing strategy
...c/main/java/org/eclipse/edc/connector/dataplane/selector/RemoteDataPlaneSelectorService.java
Outdated
Show resolved
Hide resolved
|
||
return mapper.readValue(body, tr); | ||
private <R> R handleResponse(Response response, TypeReference<? extends R> tr, R defaultValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about returning a Result<R>
here and evaluating that in client code, e.g. the addInstance
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree, given the size of the PR I added that to my TODO list, but let's see if doing that will help me solve the other 2 comments (definitely it's not gonna help me out 😅 )
...st/java/org/eclipse/edc/connector/dataplane/selector/RemoteDataPlaneSelectorServiceTest.java
Outdated
Show resolved
Hide resolved
Yes, from a control plane standpoint, that type of load balancing should be performed by the data plane "instance" itself. In other words, the control plane assumes a particular data plane instance is capable of handling those conditions. |
@paullatzelsperger solved two comments, but those tests need to be changed, because the integration scope is definitely too broad (they depend to the actual controller class!), I added that to my TODO list will do it in the next PRs. @jimmarino @paullatzelsperger so definitely the |
yep, makes sense |
To be honest, I wonder about this pull request. We volunteered to create such a DP self-registration extension. We followed your CONTRIBUTING guidelines and started different discussions on GitHub (see here or here) and on Discord (see here).
Source: @paullatzelsperger -- #2198 (reply in thread) Since this project is an Eclipse open source project which should follow the open source best practices and the Eclipse Foundation Project Handbook: Where is this change in decision discussed and documented? Why isn't it discussed in the specific discussions linked above? |
First of all, the discussions you mentioned are (as of today) a year and a half old, and in the world of Software Development this can mean a drastic change of landscape which is exactly what happened here. Furthermore, you can't seriously expect us to remember and relate all current goings-on to age-old discussions. Second, could you please provide any reference, which forces the maintainers of a project to get in contact with and align with the original authors of a previously Third, this is quite a delicate are of code, so I'm sure you understand that we want this to be implemented by be someone, who is intimately familiar with the code base and has a proven track record of stellar contributions, for example, a committer. So the chances of getting something like this accepted for someone without a track record of any sort are slim, to say the least. Lastly as I'm sure you know, since you mentioned the EF Handbook and our Contributing guidelines, it generally remains at the discretion of the committers to accept or reject any contribution based on a variety of reasons. So from my point of view, all this is perfectly aligned with OSS practices, even if you seem offended by this PR. I'm also going to lock this discussion now, because this PR is merged, so we're really beating a dead horse at this point. |
Sorry, I need to chime in here. First, you (@mhellmeier) mischaracterized what three committers (@jimmarino, @ndr-brt, and @paullatzelsperger) stated in the referenced discussions. The consensus among the committers was that any solution would need to be generalized and reconciled with work underway in the DSP specification (a.k.a. IDS). There were also requirements for idempotent behavior, which DPF was not equipped to support at the time. In the roughly year and a half since DSP has been updated to include more explicit state transitions and data formats, and the Data Plane Signaling API was introduced. Moreover, we are close to drafting a Data Plane API standard based on the latter. All of these were prerequisites to this PR, all were extensively discussed, and all have been vetted by implementation experience. This is in line with our positions expressed in the above-referenced discussions. I also don't understand how this project fails to follow Eclipse procedures. This PR was authored by a committer and was approved by two other committers. I would also have approved it, but I did not have time to finalize my review (I had no issues with it). Furthermore, the PR was the product of extensive discussions and collaboration among the committers. That's precisely how Eclipse projects should be run. Given the volume of requests and work associated with this project, it is not reasonable to expect the committers to reach out or explain their actions to every potential contributor, interested party, or person who may have raised a question in the past. In order to stay informed and up-to-date, the best approach is to attend EDC office hours, monitor discussions, and read decision records. |
I think @jimmarino and @paullatzelsperger already covered the point extensively, I just wanted to add that the related issue (#2534) remained open for like more than a year (read: IT ages :) ), so the idea to have this feature implemented was never discarded in any way, at the end it made it, so I guess it could be considered an happy ending for everyone (a committer contribution is typically less expensive in terms of time both on the "code" side and on the "review" side). |
What this PR changes/adds
Give the data-plane "self registration" capabilities, through:
data-plane-self-registration
extension, that will obtain the supported src/dest/transfer types and call the data plane selector apidata-plane-selector-control-api
extension, that offers endpoints to permit the data-planes to interact with the control-plane, particularly, they offer "registration" and "unregistration" (that will provided in a subsequent issue/PR)some diff is caused by moving stuff around and replacing the swagger plugin string with the reference to the version catalog.
Why it does that
simplify data-plane management
Further notes
transferType
already represent the handled types and it's already mandatory. in fact, the destination types field could be removed fromDataPlaneInstance
(upcoming issue)addDataplane
endpoint on the management API has been deprecated (already removed usage from E2E tests)turnCount
used for? now it isn't used by anyone, I deprecated it.signaling
api context looks to be not necessary, their api should be exposed on thecontrol
one (upcoming issue).Linked Issue(s)
Closes #2534
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.