-
Notifications
You must be signed in to change notification settings - Fork 5
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: disabling components programmatically #153
Conversation
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.
looking good, add documentation in same PR
@@ -202,7 +203,7 @@ public static class Settings { | |||
/** | |||
* Default settings for testkit. | |||
*/ | |||
public static Settings DEFAULT = new Settings("self", true, TEST_BROKER, MockedEventing.EMPTY, Optional.empty(), ConfigFactory.empty()); | |||
public static Settings DEFAULT = new Settings("self", true, TEST_BROKER, MockedEventing.EMPTY, Optional.empty(), ConfigFactory.empty(), Set.of()); |
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.
Collections.emptySet
?
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.
Any advantages? The current option is a bit shorter :)
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.
Doesn't matter here, but otherwise I would guess that emptySet is a singleton instance and the other would create a new instance.
Leave it as is.
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.
FYI, both are singletons
} | ||
|
||
/** | ||
* Disable components from running, useful for testing components in isolation. This set of disabled components will be added to {@link ServiceSetup#disabledComponents()} if configured. |
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.
are they added or replacing?
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.
Added
@@ -660,6 +664,15 @@ private final class Sdk( | |||
healthCheck = () => SdkRunner.FutureDone) | |||
} | |||
|
|||
private def isDisabled(disabledComponents: Set[Class[_]])(componentDescriptor: spi.ComponentDescriptor): Boolean = { | |||
val className = componentDescriptor.implementationName | |||
if (disabledComponents.map(_.getName).contains(className)) { |
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.
move disabledComponents.map(_.getName)
to the call site so that it's only done once, i.e.
def isDisabled(disabledComponents: Set[String]
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.
fixed
private def isDisabled(disabledComponents: Set[Class[_]])(componentDescriptor: spi.ComponentDescriptor): Boolean = { | ||
val className = componentDescriptor.implementationName | ||
if (disabledComponents.map(_.getName).contains(className)) { | ||
logger.info("Ignoring component [{}] as it is disabled in the configuration", className) |
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.
in the Setup ?
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.
Technically, this could be a Setup or Integration test configuration. Maybe I should log both options separately?
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.
probably not important to have that detail, might be enough with
"Ignoring component [{}] as it is disabled
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.
done
Co-authored-by: Patrik Nordwall <[email protected]>
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 after minor doc comments
@@ -202,7 +203,7 @@ public static class Settings { | |||
/** | |||
* Default settings for testkit. | |||
*/ | |||
public static Settings DEFAULT = new Settings("self", true, TEST_BROKER, MockedEventing.EMPTY, Optional.empty(), ConfigFactory.empty()); | |||
public static Settings DEFAULT = new Settings("self", true, TEST_BROKER, MockedEventing.EMPTY, Optional.empty(), ConfigFactory.empty(), Set.of()); |
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.
Doesn't matter here, but otherwise I would guess that emptySet is a singleton instance and the other would create a new instance.
Leave it as is.
@@ -23,6 +23,19 @@ It is important to remember that an Akka service consists of one to many distrib | |||
individually and independently, for example during a rolling upgrade. Each such instance starting up will invoke | |||
`onStartup` when starting up, even if other instances run it before. | |||
|
|||
== Disabling components | |||
|
|||
Users can use `ServiceSetup` to disable components by overriding `disabledComponents` and returning a set of component classes to disable. |
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.
"Users" is written from our (Akka's) perspective, but we should write from the reader's perspective. We are targeting developers so I think this would be better as "You can use"
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.
fixed
---- | ||
<1> Override `disabledComponents` | ||
<2> Provide a set of component classes to disable | ||
|
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.
I think we should also mention that the method can use appConfig or environment variables to control which set of components that should be disabled.
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.
Good point, added a more sophisticated code sample, should be good enough.
No description provided.