You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "GrantPSpencer (via GitHub)" <gi...@apache.org> on 2023/08/02 22:32:42 UTC
[GitHub] [helix] GrantPSpencer opened a new pull request, #2583: Fix inconsistent maintenance "reason" key formatting
GrantPSpencer opened a new pull request, #2583:
URL: https://github.com/apache/helix/pull/2583
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Fixes #2565
Formatting of the "reason" key could vary depending on how data was sent to helix-rest. This change makes it so helix-rest always checks for a "reason" key and then consistently formats it in UPPERCASE
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Checks the KV mapping passed to the helix-rest endpoint to determine if there a "reason" key present. We check for a case-insensitive key then set it to content, which then gets set as the value to the "REASON" (uppercase) key down the line.
More details can be seen in the linked issue, but we need to enforce this formatting because when this endpoint receives a non-JSON string, it sets it as the value to the "REASON" key. If the endpoint receivers a json string (KV map), then it will just use whatever key-value pairs are passed to it and does not enforce UPPERCASE formatting for the reason key. This change ensures that if a reason key is present, it will always appear as "REASON" . This is important as Java maps are case sensitive and keys of different casing are functionally two different keys and do not allow customers to easily programmatically access the reason key
### Tests
- [ ] The following tests are written for this issue:
Updated testEnableDisableMaintenanceMode() in testClusterAccessor.java to assert KV map of {"reason": "Test reason"} gets stored as {"REASON": "Test reason"}
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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] GrantPSpencer commented on pull request #2583: Fix inconsistent maintenance "reason" key formatting
Posted by "GrantPSpencer (via GitHub)" <gi...@apache.org>.
GrantPSpencer commented on PR #2583:
URL: https://github.com/apache/helix/pull/2583#issuecomment-1679288346
Pull request approved by @xyuanlu
Commit message: Fix inconsistent map key formatting in helix-rest maintenance API
--
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] junkaixue commented on a diff in pull request #2583: Fix inconsistent maintenance "reason" key formatting
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2583:
URL: https://github.com/apache/helix/pull/2583#discussion_r1283532689
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java:
##########
@@ -321,8 +322,13 @@ public Response updateCluster(@PathParam("clusterId") String clusterId,
customFieldsMap =
OBJECT_MAPPER.readValue(content, new TypeReference<HashMap<String, String>>() {
});
- // content is given as a KV mapping. Nullify content
+ // content is given as a KV mapping. Nullify content unless (case-insensitive) reason key present in map
content = null;
+ for (Map.Entry<String, String> entry : customFieldsMap.entrySet()) {
+ if ("reason".equals(entry.getKey().toLowerCase(Locale.ENGLISH))) {
Review Comment:
I think Locale.ENGLISH is not needed, right? We are not checking semantics or not.
If the key does not match, it is not right then.
--
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] desaikomal commented on a diff in pull request #2583: Fix inconsistent maintenance "reason" key formatting
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2583:
URL: https://github.com/apache/helix/pull/2583#discussion_r1293960323
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java:
##########
@@ -321,8 +321,13 @@ public Response updateCluster(@PathParam("clusterId") String clusterId,
customFieldsMap =
OBJECT_MAPPER.readValue(content, new TypeReference<HashMap<String, String>>() {
});
- // content is given as a KV mapping. Nullify content
+ // content is given as a KV mapping. Nullify content unless (case-insensitive) reason key present in map
content = null;
+ for (Map.Entry<String, String> entry : customFieldsMap.entrySet()) {
Review Comment:
just wondering: is the fast way to look if the key exists? but if we write as "reason" vs/vs "ReAsoN" then that approach won't work.
--
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] xyuanlu merged pull request #2583: Fix inconsistent maintenance "reason" key formatting
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2583:
URL: https://github.com/apache/helix/pull/2583
--
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] GrantPSpencer commented on a diff in pull request #2583: Fix inconsistent maintenance "reason" key formatting
Posted by "GrantPSpencer (via GitHub)" <gi...@apache.org>.
GrantPSpencer commented on code in PR #2583:
URL: https://github.com/apache/helix/pull/2583#discussion_r1294872371
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java:
##########
@@ -321,8 +321,13 @@ public Response updateCluster(@PathParam("clusterId") String clusterId,
customFieldsMap =
OBJECT_MAPPER.readValue(content, new TypeReference<HashMap<String, String>>() {
});
- // content is given as a KV mapping. Nullify content
+ // content is given as a KV mapping. Nullify content unless (case-insensitive) reason key present in map
content = null;
+ for (Map.Entry<String, String> entry : customFieldsMap.entrySet()) {
Review Comment:
As far as I know this is the best solution to find a key case insensitive key without making changes to the map data structure, or using something a treemap like this:
TreeMap<>(String.CASE_INSENSITIVE_ORDER);
Fortunately the map should be fairly small from what I've seen, 3-10 entries, so this inefficiency shouldn't have an impact on overall performance of the call
--
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] desaikomal commented on a diff in pull request #2583: Fix inconsistent maintenance "reason" key formatting
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2583:
URL: https://github.com/apache/helix/pull/2583#discussion_r1294875164
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java:
##########
@@ -321,8 +321,13 @@ public Response updateCluster(@PathParam("clusterId") String clusterId,
customFieldsMap =
OBJECT_MAPPER.readValue(content, new TypeReference<HashMap<String, String>>() {
});
- // content is given as a KV mapping. Nullify content
+ // content is given as a KV mapping. Nullify content unless (case-insensitive) reason key present in map
content = null;
+ for (Map.Entry<String, String> entry : customFieldsMap.entrySet()) {
Review Comment:
good to know.. thanks for working on 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] GrantPSpencer commented on a diff in pull request #2583: Fix inconsistent maintenance "reason" key formatting
Posted by "GrantPSpencer (via GitHub)" <gi...@apache.org>.
GrantPSpencer commented on code in PR #2583:
URL: https://github.com/apache/helix/pull/2583#discussion_r1283706356
##########
helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java:
##########
@@ -321,8 +322,13 @@ public Response updateCluster(@PathParam("clusterId") String clusterId,
customFieldsMap =
OBJECT_MAPPER.readValue(content, new TypeReference<HashMap<String, String>>() {
});
- // content is given as a KV mapping. Nullify content
+ // content is given as a KV mapping. Nullify content unless (case-insensitive) reason key present in map
content = null;
+ for (Map.Entry<String, String> entry : customFieldsMap.entrySet()) {
+ if ("reason".equals(entry.getKey().toLowerCase(Locale.ENGLISH))) {
Review Comment:
Habit from old team. I've updated to use equalsIgnoreCase:
if ("reason".**equalsIgnoreCase**(entry.getKey())) {
--
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