-
Notifications
You must be signed in to change notification settings - Fork 42
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
Split out signalflow into separate module #332
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.
Awesome!
<dependency> | ||
<groupId>org.apache.httpcomponents</groupId> | ||
<artifactId>httpclient</artifactId> | ||
</dependency> |
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.
That's defined as a compile dependency in the parent pom
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 the parent just sets up the versions...so this is definitely required, I confirmed that removing it breaks the build.
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 the parent just sets up the versions...so this is definitely required, I confirmed that removing it breaks the build.
I'm not saying that it's not required; I'm saying that if it's a test dep it needs <scope>test</scope>
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.
Yeah, that doesn't work because it's used by the RetryStrategy
and RetryHandler
which are production classes.
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
ready for another look @mateuszrzeszutek thanks! |
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.
🎉
This modularizes the code quite a bit. There are some tangled dependencies, so 2 additional modules also needed to be factored out:
signalfx-connection
andsignalfx-endpoint
.The dependency graph is basically:
There was one code change needed to eliminate cycles there. The
SignalFxMetricsException
which was an unnecessary (named/tag) subclass ofRuntimeException
was replaced with justRuntimeException
.While I do think this split helps to better organize and show where dependencies are and to eventually pull out signalflow, it also highlights that signalflow still has some dependencies on this common stuff.