Fix race condition while reading chunks

[jotak]

Metrics reading could occur while writing chunks is not finished (ie. some chunks are written but not all), which would produce errors. We can detect that and return nil instead.

[jotak]

Note: this error has been seen once on the java agent integration tests (and PR sent there), but not (yet) on the ruby client, though it could happen here also in theory.

[josejulio]

@jotak Rubocop is complaining about some code, could you please update it?

[jotak]

oops, forgot to run it locally, sorry... will update

[jotak]

@josejulio the test failures are then same that were mentioned by @pilhuhn yesterday, did you find out what's the cause?

[josejulio]

Some tests were pointing directly to subsystem agent, latest version of hawkular services uses the hawkular javaagent. I updated the tests on #218

[josejulio]

@jotak could you please rebase?

[jotak]

done

[coveralls]

Coverage increased (+0.04%) to 95.977% when pulling c76916b6ad34d1ab70e886431c580b9754c0ce9f on jotak:fix-test-racecond into 52ace36c155adddab7c2c36fdd25beeeea7ff019 on hawkular:master.

[josejulio]

Thanks!

[josejulio]

Sorry for not asking until now, but could you put a (unit) test for it?

[jotak]

@josejulio I'm adding a new file for unit tests (there was only integration test on that til now). Also changed rebuild_from_chunks to be a class method.

[coveralls]

Coverage increased (+0.04%) to 95.971% when pulling 00d2f9af7384d372f08a6990cefc80e1833bf3be on jotak:fix-test-racecond into 52ace36c155adddab7c2c36fdd25beeeea7ff019 on hawkular:master.

[josejulio]

Awesome thanks!

[jkremser]

@josejulio is this good to go?

[coveralls]

Coverage decreased (-17.3%) to 78.613% when pulling 0605b506c5eeb6f454080135653c5d5b3958b030 on jotak:fix-test-racecond into d9a5e86201b884819c6cfb13d049c81e6bab2760 on hawkular:master.

[jotak]

Hmm, still some work to do here...

[jkremser]

@jotak perhaps it doesn't have to be a class method, but only normal instance method htat is private?

[jotak]

I made it a class method because it has no dependency on the object's state, it looks cleaner to me but also easier to test with unit tests (the issue, if I have to instanciate the client in the unit test, is that the initializer performs http call to get the version... which sucks for a unit test)

Private class methods cannot be called from an instanciated object of that class, such as in java?

[jkremser]

I see, I tried to make it instance private (https://pastebin.com/61bFZMG2) and the old tests passed, but you are right that initializing the client does the call.

Edit: perhaps it would worth adding some option in the hash that is passed to the client constructor that would omit the version check. But this is not related to your PR at all, I don't want to block it, on the other hand exposing internal method is no good either, because we would need to maintain them and preserve their backward-compatibility, even though they are only impl. detail

[jotak]

That's probably a common case for java-ist doing ruby :) http://stackoverflow.com/questions/20674/is-there-a-way-to-call-a-private-class-method-from-an-instance-in-ruby

Well, if no objection, I'd rather go for making it public.

[jkremser]

I'd rather go for making it public.

:+1: go ahead, I can take that in a follow up PR by adding option to the client that will not do the http check.

[jotak]

Ok. But intuitively I would say it's a good practice to turn stateless methods into class methods, no? It's a way to formalize that they don't rely on the object state.

[coveralls]

Coverage increased (+0.04%) to 95.971% when pulling 401dbfc364e3c4c7330433a998c28d0046b99f4e on jotak:fix-test-racecond into d9a5e86201b884819c6cfb13d049c81e6bab2760 on hawkular:master.