Added version to the REST API

[lstav]

Added Tserver Accumulo version to the REST API as discussed in #280. This does not display the version on the Monitor, but it can be seen when making the REST api call. I can add another column to display it if people want it.

[joshelser]

Add a test please? ;)

[ctubbsii]

@joshelser Are you asking for a test to ensure that Jackson serializes the included value to JSON correctly?

[joshelser]

Are you asking for a test to ensure that Jackson serializes the included value to JSON correctly?

No, I wouldn't ask Luis to test another project in Accumulo.

However, I have no confidence that this change does anything. If this isn't possible, explaining why is a legitimate response as always...

[ctubbsii]

I was just confused about what kind of test you were thinking of, and hoping you could clarify. This change only adds a field to a POJO for the sole purpose of including it in the REST endpoint's JSON produced by Jackson.

I'm not saying it's not possible to test for it... but some hint as to what kind of test you thought should be added might be useful.

[ctubbsii]

Starting up monitor and ran:
curl http://localhost:9995/rest/tservers | jq .
Output shows version field, as expected.

[lstav]

Like @ctubbsii, I used curl (minus the jq cause I'm not that fancy :P) to verify that the Version showed as a field (with "2.0.0-SNAPSHOT" being the value).

[joshelser]

curl http://localhost:9995/rest/tservers | jq .

Sounds to me exactly the sort of thing we could add as a unit test.

[ctubbsii]

I think such a test really only verifies that Jackson is working properly.

I suppose it's reasonable to write an integration test for the REST API, but it seems to me that should be done in a comprehensive way, rather than as a series of one-offs for specific fields in specific endpoints. Could be a follow on issue. Will create a JIRA for it, so it can be done as follow-on, but I think it's fine to merge this in now.

[joshelser]

I think such a test really only verifies that Jackson is working properly.

I find it very humorous that you both ran the same test to verify that the change worked but don't want to write a test to do it automatically for you. I guess that's just me. Carry on.

[ctubbsii]

I didn't say I didn't want to have it tested automatically. I just said I'd prefer to have the test coverage more comprehensive. My reasoning is that our current test suite takes 4 hours to run, and that's largely due to having hundreds of one-off tests, rather than more thoughtful test implementations. I've created ACCUMULO-4683 to add comprehensive testing to the REST API which would cover this.

[ctubbsii]

Actually, upon second thought... I think I have an idea for a reasonable, fast-running test. We can write a test that verifies that the REST POJO is properly constructed from the TabletServerStatus thrift object. Would that be more like the test you were thinking of, @joshelser ?

[joshelser]

My reasoning is that our current test suite takes 4 hours to run, and that's largely due to having hundreds of one-off tests, rather than more thoughtful test implementations.

That's a reason to clean up the tests we have, not a reason to not write new tests. Cutting of your nose to spite your face, if you ask me.

Would that be more like the test you were thinking of

I was expecting testing along the lines of what you described in ACCUMULO-4683, but I missed the opportunity to put my foot down in the first place, I guess.

What you have described is better than nothing, so I'm in favor of that.

[ctubbsii]

Rebase'd, added test, fixed bugs found by test, and pushed. :)

[lstav]

Thanks for the help @ctubbsii!!