-
Notifications
You must be signed in to change notification settings - Fork 157
Br add status #104
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
Br add status #104
Conversation
… testing and think if the interface is sane or not.
…ce to return an Iterable<NodeStats) rather than an interator.
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'm sure this is just a product of your IDE, but mixing these non-functional changes in with features makes reviewing harder. Personally I prefer the fully qualified import statements, but that is discussion for another time. For now I think it is not the best idea to unilaterally change style like this and mix it in with functional/feature changes.
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.
It IS, and I fixed that like 3 times because it kept doing it - I obviously missed that one. I also don't care for wholesale imports.
|
Tests pass and functionally I'm +1, but I think we need to discuss the minor style issues. I also think that using the legacy Crockford JSON lib is a mistake. |
|
I'm happy to fix the style issues, I should have caught them and will only take a few mins. Agreed on the JSON lib, I used it because it was already there but would prefer to use Jackson. I'll make the changes later. |
- Fixed style / formatting - Converted to using Jackson object mapping - eliminated http.response.StatsResponse and its tests as it was no longer necessary
|
Ok - fixed the style issues, converted over to using Jackson object mapping. |
|
As discussed, test fails against 1.1, adding @JsonIgnoreProperties(ignoreUnknown=true) makes it pass, but maybe we need to support the 1.1 stats. |
…hose that are currently broken and containing the string "undefined" rather than an int value
|
I did both, actually. All the new stats are added and any new ones won't cause the client to break. That being said, a few stats are broken on the Riak side and contain the string "undefined" rather than an integer value. I've coded around this at the moment with a custom deserializer but they should be fixed in Riak |
|
Looks good. All tests pass. +1 to merge. Nice work! Recommend you raise bug against riak_kv for the 'undefined' stats. |
Adds the /stats command to the client. This is of course not supported via protocol buffers.
Easy example is:
HTTPClusterConfig c = new HTTPClusterConfig(10);
HTTPClientConfig cc = HTTPClientConfig.defaults();
c.addHosts(cc,"127.0.0.1:8098","127.0.0.1:8098");
final IRiakClient riakClient = RiakFactory.newClient(c);
for (NodeStats ns : riakClient.stats())
{
System.out.println(ns.nodename());
}