You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@griffin.apache.org by GitBox <gi...@apache.org> on 2020/11/17 03:51:46 UTC
[GitHub] [griffin] XiaoyuBD opened a new pull request #587: Support http remote conf
XiaoyuBD opened a new pull request #587:
URL: https://github.com/apache/griffin/pull/587
In our production practice, many Griffin jobs run on yarn in cluster mode. We upload different conf files to the http file server and we also provide services that generate specific configurations based on different HTTP URLs.
So we supports setting HTTP URLs as conf in submitting Griffin jobs in this PR. There is no effect on JSON or File conf mode. And it works well in our production environment for a long time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [griffin] XiaoyuBD commented on pull request #587: Support http remote conf
Posted by GitBox <gi...@apache.org>.
XiaoyuBD commented on pull request #587:
URL: https://github.com/apache/griffin/pull/587#issuecomment-730375557
@wankunde Can you check this PR, please? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [griffin] XiaoyuBD commented on pull request #587: Support http remote conf
Posted by GitBox <gi...@apache.org>.
XiaoyuBD commented on pull request #587:
URL: https://github.com/apache/griffin/pull/587#issuecomment-738654894
@wankunde Thank you for the review, it is really strange to add getData to HttpUtil.
Also, I checked the code in HttpUtil and found the following problems:
1. The `HttpUtil.postData()` and `HttpUtil.httpRequest()` has no usages.
2. There is a bug in `HttpUtil.doHttpRequest()`. When method = 'PUT', after `case PUT_REGEX() =>`, the GET request is executed instead of PUT.
3. The third parameter `params: Map[String, Object]` in `HttpUtil.doHttpRequest()` are not used and do not take effect.
4. `HttpUtil.doHttpRequest()` only supports POST and "PUT" request. (GET and DELETE are not supported.)
So my new commit is as follows:
1. Remove `HttpUtil.postData()` and `HttpUtil.httpRequest()`, which has no usages.
2. Reimplement` HttpUtil.doHttpRequest()`, enable use of third parameter `params: Map[String, Object]`, and support GET, POST, PUT, DELETE methods, and return status code and response string.
3. ElasticSearchSink.scala, which calls HttpUtil.doHttpRequest(), has also been modified accordingly.
Since the method is an external Http request, UT is not easy to implement. I tested it in our environment and it worked very well.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [griffin] XiaoyuBD commented on pull request #587: Support http remote conf
Posted by GitBox <gi...@apache.org>.
XiaoyuBD commented on pull request #587:
URL: https://github.com/apache/griffin/pull/587#issuecomment-734046051
@wankunde Thanks for the reminder.
The code mis-formatted error has been corrected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [griffin] wankunde commented on pull request #587: Support http remote conf
Posted by GitBox <gi...@apache.org>.
wankunde commented on pull request #587:
URL: https://github.com/apache/griffin/pull/587#issuecomment-736974667
Hi, @XiaoyuBD
Thanks for your contribution.
In my option, `doHttpRequest` should support `GET`, `POST`, `PUT`, `DELETE` http methods, and return http status code and response string.
It seems strange to add another method called `getData`.
What do you think ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [griffin] wankunde commented on pull request #587: Support http remote conf
Posted by GitBox <gi...@apache.org>.
wankunde commented on pull request #587:
URL: https://github.com/apache/griffin/pull/587#issuecomment-739647875
@XiaoyuBD
LGTM, thanks for your contribution.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [griffin] wankunde commented on pull request #587: Support http remote conf
Posted by GitBox <gi...@apache.org>.
wankunde commented on pull request #587:
URL: https://github.com/apache/griffin/pull/587#issuecomment-732922096
Hi, @XiaoyuBD
The CI failed due to scala code style check.
You can see the detail message with `mvn scalastyle:check -pl measure` command.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [griffin] asfgit closed pull request #587: Support http remote conf
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #587:
URL: https://github.com/apache/griffin/pull/587
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org