-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add ProjectResolver
to RemoteClusterService
constructor
#133006
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
Add ProjectResolver
to RemoteClusterService
constructor
#133006
Conversation
Follow up to work done in elastic#131894 to make RemoteClusterService multi-project aware. This task plumbs the ProjectResolver down into the RemoteClusterService from NodeConstruction. Resolves: ES-12572
There are elasticsearch multi-project tests that are not using the default Example test:
Test config:
|
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.
For the YAML test failures, the short answer is that we can mute them in the associated build.gradle file. Please also add a comment to explain why the tests need to be muted.
The issue is that settings update consumers are invoked in a context without project. How to notify components for per-project setting changes is still an open issue (ES-11463). We might be able to fix the tests once the issue is resolved. Technically it may be out of scope for CPS if it only supports config via ProjectCustom. In that case, these tests will simply be incompatible with multi-project. If the same test coverage is critical, we (the ES team not just distrib-coord) will need to add a version that uses the ProjectCustom so that it can be tested in MP setup on the serverless side.
// NOTE: Only for use in tests | ||
public TransportService( |
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.
Nit: Convention for this kind of comment is something like "public for testing"
I was running into another test failure (see below). Is the assertion too strict given the current state of the codebase in terms of multi-project support? In 69a704b I replaced the assertion with a
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
OK that's unfortunate but seems to be the best course of action right now. The reason is that ML code is larget not MP ready. More specifically, it should a |
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.
LGTM
Follow up to work done in #131894 to make
RemoteClusterService
multi-project aware. This task plumbs theProjectResolver
down into theRemoteClusterService
fromNodeConstruction
.Resolves: ES-12572