Allow AtomixCluster to be autowired everywhere #10354
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
Instead of having multiple Atomix factories, it makes more sense to simply eschew going through layers of intermediate configuration classes and builders, and simply have each application map its configuration to the general Atomix `ClusterConfig`, from which we can create a cluster easily. This will greatly simplify injecting the cluster in the broker later on.
2 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import org.springframework.core.env.PropertiesPropertySource; | ||
| import org.springframework.core.env.StandardEnvironment; | ||
|
|
||
| final class WorkingDirectoryConfigurationTest { |
Check notice
Code scanning / CodeQL
Unused classes and interfaces
|
Super cool, since the SystemContext is in charge of initializing the configuration in tests, most tests fail to build a cluster (since we want to inject the cluster). |
npepinpe
force-pushed
the
9996-atomix-cluster
branch
4 times, most recently
from
0086464
to
53469ef
Compare
10 tasks
10 tasks
deepthidevaki
approved these changes
Sep 15, 2022
atomix/cluster/src/main/java/io/atomix/cluster/discovery/BootstrapDiscoveryProvider.java
Outdated
Show resolved
Hide resolved
atomix/cluster/src/main/java/io/atomix/cluster/protocol/SwimMembershipProtocol.java
Outdated
Show resolved
Hide resolved
dist/src/main/java/io/camunda/zeebe/broker/WorkingDirectoryConfiguration.java
Outdated
Show resolved
Hide resolved
dist/src/main/java/io/camunda/zeebe/broker/BrokerClusterConfiguration.java
Show resolved
Hide resolved
|
Yes, sorry, this was much more changes than I wanted, and I had to find a balance between fully moving everything shared between broker/gateway to dist and not creating too much work. At the same time, it highlighted that if we will do more with Spring, we may want to learn more about how it works 😄 |
Resolve the working directory via configuration, as it will be required to initialize the messaging configuration for `AtomixCluster` down the line.
Refactors to allow the cluster services to be created outside of the broker, and injected directly, though not yet started. This reduces the cluster step to just starting the cluster. To simplify testing, temporary factories are added, mostly because many tests expect to be able to modify the cluster services by modifying the `BrokerCfg`. Most of `ClusterConfigFactory` should be transferable to the `dist` module once this is not true anymore.
npepinpe
force-pushed
the
9996-atomix-cluster
branch
from
53469ef
to
41bed32
Compare
npepinpe
force-pushed
the
9996-atomix-cluster
branch
from
152b82f
to
870286d
Compare
|
bors merge |
|
Build succeeded: |
zeebe-bors-camunda bot
added a commit
that referenced
this pull request
Sep 15, 2022
10357: Allow reuse of shared actuators depending on BrokerClient r=npepinpe a=npepinpe ## Description This PR provides the last remaining bit to #9996 by having each application provide a `BrokerClient` bean, allowing us to create shared actuators which do not need any bridge. To showcase, the `RebalancingService` was collapsed into a single class. ## Related issues closes #9996 depends on #10354 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
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.
Description
This PR extracts building the
AtomixClusterfrom the broker into thedistmodule. Unfortunately, because many of our testing utilities break all kinds of abstraction, and our tests still heavily rely on modifying the cluster via theBrokerCfg, mapping theBrokerCfgto theAtomixClusteris still done in thebrokermodule. However, everything is prepared to eventually move this out to the dist module itself, such that our configuration would simply instantiate the AtomixClusterConfigtype, and later inject aAtomixClusterin the broker. See theStandaloneGatewayas an example; there is plenty of opportunity to DRY it up once both are simply configuringClusterConfigdirectly, and it will remove intermediate layers of builders/config.In order to achieve that at the moment, since the network configuration relies on being initialized with the right broker base path, I also had to pull up the working directory into its own configuration, such that it can be autowired as well. This is a little more abstraction than I'd like, but it's due to how the
BrokerCfgis still the central point for all the components, even those not specific to the broker.The broker configuration is thus not initialized in the system context or broker anymore, but is given to the broker already initialized. This is done via the Spring configuration
BrokerConfiguration. It uses two beans to do this, which I dislike, but I'm not sure what's the best way to do this. I generally dislike that the configuration must be initialized anyway, it's very brittle and easy to forget, so I'd rather figure a way to avoid this in the future. I would propose a follow up which removes this, and instead enforces that we create the broker config either passing the base already, or using a factory method which does this. I can also do it here, but it will increase how much changes are here.I think in the future the cluster services should be started before the broker itself, but we can figure this out later. For now, it means that if the actuator wants to make use of them before the broker finishes starting, it will fail - this is something we can handle semi-gracefully, so I would leave it as is.
Related issues
related to #9996
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.