You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2022/09/20 21:13:39 UTC
[GitHub] [helix] micahstubbs opened a new pull request, #2222: WIP 2221/config update 500
micahstubbs opened a new pull request, #2222:
URL: https://github.com/apache/helix/pull/2222
### Description
<!-- Write a concise description: "what?, why?, how?" and then add some details about this PR, including screenshots of any UI changes -->
<!-- This PR fixes this Helix issue & includes the Helix issue in the PR description. Link your issue number here: You can write `Fix #123`. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue -->
This WIP PR ensures that the response send to helix-rest from helix-front is identical to a working curl request.
Here is the desired payload format, modeled after the working curl request:
```json
{"id":"my-test-cluster","mapFields":{"TEST_FIELD":{"test_sub_field": 18}}}
```
Here is the bug format seen before, which differed slightly from the curl request:
```json
{
"id":"my-test-cluster",
"mapFields":{"TEST_FIELD":{"test_sub_field": "18"}},
"listFields": {},
"simpleFields": {}
}
```
Notice that properties with empty object values are included and that the number value of `"test_sub_field"` is a string, not a number.
Fix #2221
### Tests
<!-- List the names of new unit or integration tests -->
No new tests yet. Will add a test if this proves to be a frontend issue.
### Code Style
<!-- Ensure the PR diff has been formatted using [Prettier](https://prettier.io) -->
Formatted using [Prettier](https://prettier.io)
<!-- ### Changes that Break Backward Compatibility (Optional)
- [ ] My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include: -->
<!-- Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method or API behavior. -->
<!-- ### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page: -->
<!-- Link the GitHub wiki you added -->
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] somecodemonkey commented on a diff in pull request #2222: 2221/config update 500
Posted by GitBox <gi...@apache.org>.
somecodemonkey commented on code in PR #2222:
URL: https://github.com/apache/helix/pull/2222#discussion_r977039874
##########
helix-front/server/controllers/helix.ts:
##########
@@ -50,9 +56,9 @@ export class HelixCtrl {
};
request[method](options, (error, response, body) => {
if (error) {
- res.status(500).send(error);
+ res.status(response.statusCode || 500).send(error);
Review Comment:
Is there ever a case where the statusCode will be empty?
##########
helix-front/server/controllers/helix.ts:
##########
@@ -41,6 +42,12 @@ export class HelixCtrl {
if (apiPrefix) {
const realUrl = apiPrefix + url.replace(`/${helixKey}`, '');
+ console.log('realUrl from helix api proxy');
+ console.log(realUrl);
+ console.log('');
+ console.log('request body');
+ console.log(req.body);
Review Comment:
Not against this but one option here is to gate this behind a local storage toggle for debugging. It would prevent unnecessarily logging at all times.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] NealSun96 merged pull request #2222: 2221/config update 500
Posted by GitBox <gi...@apache.org>.
NealSun96 merged PR #2222:
URL: https://github.com/apache/helix/pull/2222
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] micahstubbs commented on a diff in pull request #2222: 2221/config update 500
Posted by GitBox <gi...@apache.org>.
micahstubbs commented on code in PR #2222:
URL: https://github.com/apache/helix/pull/2222#discussion_r977041949
##########
helix-front/server/controllers/helix.ts:
##########
@@ -50,9 +56,9 @@ export class HelixCtrl {
};
request[method](options, (error, response, body) => {
if (error) {
- res.status(500).send(error);
+ res.status(response.statusCode || 500).send(error);
Review Comment:
Good call, will handle 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] micahstubbs commented on a diff in pull request #2222: WIP 2221/config update 500
Posted by GitBox <gi...@apache.org>.
micahstubbs commented on code in PR #2222:
URL: https://github.com/apache/helix/pull/2222#discussion_r976851252
##########
helix-front/server/controllers/helix.ts:
##########
@@ -41,6 +42,12 @@ export class HelixCtrl {
if (apiPrefix) {
const realUrl = apiPrefix + url.replace(`/${helixKey}`, '');
+ console.log('realUrl from helix api proxy');
+ console.log(realUrl);
+ console.log('');
+ console.log('request body');
+ console.log(req.body);
Review Comment:
I'd like to leave this logging in the server logs, to aid debugging similar issues in the future. I'm happy to modify the formatting if reviewers recommend 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] micahstubbs commented on pull request #2222: 2221/config update 500
Posted by GitBox <gi...@apache.org>.
micahstubbs commented on PR #2222:
URL: https://github.com/apache/helix/pull/2222#issuecomment-1256721063
This PR is ready to be merged, approved by @somecodemonkey
Final commit message:
## Ensure request body is sent from helix-front to helix-rest (#2221 )
Fix Angular http payload argument bug
Display helix-rest status codes in Helix UI
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org