You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2021/10/28 13:27:00 UTC
[GitHub] [drill] CuteKittyhoho opened a new pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
CuteKittyhoho opened a new pull request #2353:
URL: https://github.com/apache/drill/pull/2353
# [DRILL-7983](https://issues.apache.org/jira/browse/DRILL-7983): Add a REST API to support the get running or completed profiles
## Description
Drill have a REST API to get the profiles of running and completed queries. this goal of ticket is extended from the `/profiles.json` to support get the running or completed profiles (not all of them). the client does not need to filter with the state.
old URI
```
/profiles.json
```
new URI
```
/profiles.json
/profiles/running
/profiles/completed
```
## Documentation
NA
## Testing
Manual tested (use Postman).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] dzamo commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
dzamo commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-978917466
+1 for the proposal from @paul-rogers to use a URL query option on /queries.json to implement this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-955150025
Overall, looks fine. I wonder, can we add a real test? Historically, the REST API stuff never had tests. I ended up writing a bunch when I redid the storage plugin layer because I didn't have the patience to test things with Postman over and over. Can we do something like that here? To test, there is one test config flag to set to enable the HTTP server: it is normally disabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-957641875
@CuteKittyhoho
Here's an example of some tests for some unit tests for the REST API. Could you please use this as a template for some unit tests for this PR?
Thanks!
https://github.com/paul-rogers/drill/blob/02365eac3ec68176029f05149a6062fc2af8bb21/exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/TestRestJson.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-966278842
@CuteKittyhoho
Did you see the review comments from Paul? I also think this would be ok to merge, but some additional unit tests would be nice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] CuteKittyhoho commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
CuteKittyhoho commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-988608321
@paul-rogers @cgivre Thanks again for your good advice! I have finished task of real test now. It first issued two queries then checked the results from ```/profiles.json```, ```/profiles/running``` and ```/profiles/completed``` respectively. Adding a status parameter in GET request such as ```/profiles/json?status=[all|running|completed]``` is also done, so could you please review these things now? However, implementing a test with threads to check running queries is a bit of challenging, so it still need some time for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] paul-rogers commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-976015866
@CuteKittyhoho, thanks for this PR. I wonder, can we simplify the API a bit. As you stated your change is to change the original `/profiles.json` to:
```text
/profiles.json
/profiles/running
/profiles/completed
```
One could imagine other filters as well: failed requests, completed requests, running requests by Charles, etc.
So, I wonder if it makes sense to add the filter as a query option:
`/profiles/json?status=[all|running|completed]&...`
Where the `...` is whatever someone else wants to add later.
Might make the code a bit simpler also.
Then, on the unit tests: testing this one might be hard: there is no good way to ensure that there are, in fact, running queries when you request them. If it were me, I'd add a hack to the mock data source. Add a global lock. By default, it is unlocked so existing code works. A test can set the lock, then launch a query in one thread, while testing your API in another. The mock data source, when it fetches a batch, would block if the lock is set, waiting for the lock to be cleared. Ugly, but it should work.
Now, I'm not sure it's fair for us to ask you to add all that for just this case, so I guess I'm OK with doing only manual ad-hoc tests for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-957641875
@CuteKittyhoho
Here's an example of some tests for some unit tests for the REST API. Could you please use this as a template for some unit tests for this PR?
Thanks!
https://github.com/paul-rogers/drill/blob/02365eac3ec68176029f05149a6062fc2af8bb21/exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/TestRestJson.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-957641875
@CuteKittyhoho
Here's an example of some tests for some unit tests for the REST API. Could you please use this as a template for some unit tests for this PR?
Thanks!
https://github.com/paul-rogers/drill/blob/02365eac3ec68176029f05149a6062fc2af8bb21/exec/java-exec/src/test/java/org/apache/drill/exec/server/rest/TestRestJson.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre edited a comment on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
cgivre edited a comment on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-966278842
@CuteKittyhoho
Did you see the review comments from Paul? I also think this would be ok to merge, but some additional unit tests would be nice.
Also, can you please rebase on current master and resolve merge conflicts? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] CuteKittyhoho commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
CuteKittyhoho commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-986613446
Brilliant idea! An URL with arguments will make things far better, I wanna have a try about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] CuteKittyhoho commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
CuteKittyhoho commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-967986882
@cgivre Thanks for your review! Resolving merge conflicts is in progress. After finishing this, I will go and undertake unit tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] dzamo commented on pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
dzamo commented on pull request #2353:
URL: https://github.com/apache/drill/pull/2353#issuecomment-983645065
Dear PR author and reviewers.
This is a generic message to say that we would like to merge this PR in time for the 1.20 release. Currently we're targeting a master branch freeze date of 2021-12-10 (10 Dec). Please strive to complete development and review by this time, or indicate that the PR will need more time (and how much).
Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] dzamo merged pull request #2353: DRILL-7983: Add a REST API to support the get running or completed profiles
Posted by GitBox <gi...@apache.org>.
dzamo merged pull request #2353:
URL: https://github.com/apache/drill/pull/2353
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org