[GitHub] accumulo pull request #282: Added version to the REST API

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #282: Added version to the REST API

ctubbsii
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 <[hidden email]>
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #282: Added version to the REST API

ctubbsii
In reply to this post by ctubbsii
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...