You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by lstav <gi...@git.apache.org> on 2017/07/21 20:37:22 UTC
[GitHub] accumulo pull request #282: Added version to the REST API
GitHub user lstav opened a pull request:
https://github.com/apache/accumulo/pull/282
Added version to the REST API
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lstav/accumulo restEndpoint
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/282.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #282
----
commit c4d70f313b0ed4083b2bb789c21197e6d2a40dff
Author: Luis Tavarez <ze...@outlook.com>
Date: 2017-07-21T20:31:48Z
Added version to the REST API
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/282
Rebase'd, added test, fixed bugs found by test, and pushed. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/282
> `curl http://localhost:9995/rest/tservers | jq .`
Sounds to me exactly the sort of thing we could add as a unit test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by lstav <gi...@git.apache.org>.
Github user lstav commented on the issue:
https://github.com/apache/accumulo/pull/282
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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/282
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/282
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/282
> 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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/282
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by lstav <gi...@git.apache.org>.
Github user lstav commented on the issue:
https://github.com/apache/accumulo/pull/282
Thanks for the help @ctubbsii!!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/282
> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/282
> 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 similar change to do it automatically for you. I guess that's just me. Carry on.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #282: Added version to the REST API
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/282
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/282
@joshelser Are you asking for a test to ensure that Jackson serializes the included value to JSON correctly?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/282
Add a test please? ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #282: Added version to the REST API
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/282
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 ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---