Skip to content
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

Upgrade to metrics library v3.0.1 #193

Closed
wants to merge 4 commits into from
Closed

Upgrade to metrics library v3.0.1 #193

wants to merge 4 commits into from

Conversation

rore
Copy link

@rore rore commented Oct 31, 2013

These are the changes required to upgrade the metrics library to the new version (3.0.1).
The new metrics version includes changes in namespace and interfaces.

@cloudbees-pull-request-builder

Hystrix-pull-requests #55 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Contributor

Is this a backwards compatible change, or do we need a separate module?

@benjchristensen
Copy link
Contributor

Thanks by the way for contributing!

@rore
Copy link
Author

rore commented Oct 31, 2013

Ah, you are right. It's not backward compatible.
Since the new metrics version is on another namespace there's no problem supporting both. My changes just needs to be duplicated to other classes and the existing ones can remain.
I can get to it in the next couple of days.

@benjchristensen
Copy link
Contributor

Thanks.

@rore
Copy link
Author

rore commented Nov 3, 2013

Done. The previous code is unchanged, I added the new classes using the v3 metrics version under a new namespace.

@cloudbees-pull-request-builder

Hystrix-pull-requests #57 FAILURE
Looks like there's a problem with this pull request

@@ -5,6 +5,7 @@ apply plugin: 'idea'
dependencies {
compile project(':hystrix-core')
compile 'com.yammer.metrics:metrics-core:2.2.0'
compile 'com.codahale.metrics:metrics-core:3.0.1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay to have both versions imported in the same project? Are they namespaced separately?

Or do we need a completely new submodule 'hystrix-yammer-metrics3-publisher'?

And what is the "official" GroupID since there is both 'com.yammer' and 'com.codahale'. Version 2 was 'com.yammer', this adds version 3 using 'com.codahale'. Both are available on Maven Central: http://search.maven.org/#search%7Cga%7C1%7Cmetrics-core

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In version 3 they changed the namespace from com.yammer to com.codahale to allow side-by-side, so there is no problem with having them both in the same project. No need for a separate sub-module.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that does mean we are forcing people to pull in both dependencies, version 2 and version 3.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can exclude the one you don't need in your pom so it won't bring it.
Not good enough?

On Tue, Nov 12, 2013 at 9:41 PM, Ben Christensen
[email protected]:

In hystrix-contrib/hystrix-yammer-metrics-publisher/build.gradle:

@@ -5,6 +5,7 @@ apply plugin: 'idea'
dependencies {
compile project(':hystrix-core')
compile 'com.yammer.metrics:metrics-core:2.2.0'

  • compile 'com.codahale.metrics:metrics-core:3.0.1'

But that does mean we are forcing people to pull in both dependencies,
version 2 and version 3.


Reply to this email directly or view it on GitHubhttps://github.com//pull/193/files#r7604061
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's rather awkward to require people to manually exclude libraries they don't want to pull in.

Especially since it's a new namespace, how about a new module 'hystrix-codahale-metrics3-publisher'?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds decent :-)

On Tue, Nov 12, 2013 at 9:59 PM, Ben Christensen
[email protected]:

In hystrix-contrib/hystrix-yammer-metrics-publisher/build.gradle:

@@ -5,6 +5,7 @@ apply plugin: 'idea'
dependencies {
compile project(':hystrix-core')
compile 'com.yammer.metrics:metrics-core:2.2.0'

  • compile 'com.codahale.metrics:metrics-core:3.0.1'

That's rather awkward to require people to manually exclude libraries they
don't want to pull in.

Especially since it's a new namespace, how about a new module
'hystrix-codahale-metrics3-publisher'?


Reply to this email directly or view it on GitHubhttps://github.com//pull/193/files#r7604748
.

@caps
Copy link

caps commented Dec 18, 2013

Hey guys, this is something I'm interested in using. Is there anything I can do to help out? e.g. Create the new module 'hystrix-codahale-metrics3-publisher'?

@cloudbees-pull-request-builder

Hystrix-pull-requests #61 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

Hystrix-pull-requests #62 FAILURE
Looks like there's a problem with this pull request

@rore
Copy link
Author

rore commented Dec 18, 2013

Sorry for the delay, was a bit busy.
I moved the metrics3 changed code into a separate project.

@caps
Copy link

caps commented Dec 18, 2013

No worries! Let me know if there is anything I can do to assist.

@benjchristensen
Copy link
Contributor

I believe #199 achieves this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants