You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/05/13 18:37:10 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2109: Apply replaceAll to all parts of JSON strings
milleruntime opened a new pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109
* Apply replaceAll for quotes of all parts of the default JSON values in
Property.java so they are displayed as valid JSON
* Closes #2105
--
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] [accumulo] milleruntime commented on pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#issuecomment-840814476
@ctubbsii this bug makes me wonder if there is an issue with the autoformatter. Is it possible that the line was coded like this:
<pre>
"{'name':'medium','maxSize':'128M','numThreads':2},{'name':'large','numThreads':2}]".replaceAll("'", "\""),
</pre>
But was automatically split, changing the output:
<pre>
"{'name':'medium','maxSize':'128M','numThreads':2},"
+ "{'name':'large','numThreads':2}]".replaceAll("'", "\""),
</pre>
--
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] [accumulo] milleruntime commented on pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#issuecomment-841406884
I did some experimenting and tie formatter does not break up the string.
--
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] [accumulo] keith-turner commented on a change in pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#discussion_r632117210
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -458,8 +458,8 @@
"The maximum number of files a compaction will open"),
TSERV_COMPACTION_SERVICE_META_EXECUTORS(
"tserver.compaction.major.service.meta.planner.opts.executors",
- "[{'name':'small','maxSize':'32M','numThreads':2},"
- + "{'name':'huge','numThreads':2}]".replaceAll("'", "\""),
+ ("[{'name':'small','maxSize':'32M','numThreads':2}," + "{'name':'huge','numThreads':2}]")
Review comment:
If the string fits on a single line could remove the concatenation. Leaving the parens in case there is a bug w/ the formatter as you mentioned.
```suggestion
("[{'name':'small','maxSize':'32M','numThreads':2},{'name':'huge','numThreads':2}]")
```
--
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] [accumulo] ctubbsii commented on pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#issuecomment-840865048
I don't believe the formatter ever creates a string concatenation in order to break up a long line. This was most likely done manually by somebody trying to make it compliant with a checkstyle rule to check the line length, and they simply overlooked the fact that the method call was only being done on the last component rather than the whole String. As Keith mentioned, you could keep the parentheses in there, in case it does get wrapped or split later.
--
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] [accumulo] ctubbsii commented on a change in pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#discussion_r632027010
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -442,7 +442,7 @@
"The maximum number of files a compaction will open"),
TSERV_COMPACTION_SERVICE_ROOT_EXECUTORS(
"tserver.compaction.major.service.root.planner.opts.executors",
- "[{'name':'small','maxSize':'32M','numThreads':1},"
+ "[{'name':'small','maxSize':'32M','numThreads':1},".replaceAll("'", "\"")
+ "{'name':'huge','numThreads':1}]".replaceAll("'", "\""),
Review comment:
Could do this instead (I think this was probably the original intent of the replaceAll, but probably got overlooked when the strings were broken up in order to wrap them):
```suggestion
("[{'name':'small','maxSize':'32M','numThreads':1},"
+ "{'name':'huge','numThreads':1}]").replaceAll("'", "\""),
```
--
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] [accumulo] milleruntime merged pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109
--
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] [accumulo] milleruntime edited a comment on pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
milleruntime edited a comment on pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#issuecomment-841406884
I did some experimenting and the formatter does not break up the string.
--
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] [accumulo] ctubbsii commented on pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#issuecomment-840760658
For what it's worth, Java 15 (13 preview) adds a TextBlock feature that would make it easier to have double quotes inside a string, without a ton of escaping. It would also make it easier to format our JSON in our String literals in a more readable way.
--
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] [accumulo] ctubbsii commented on a change in pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#discussion_r632027010
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -442,7 +442,7 @@
"The maximum number of files a compaction will open"),
TSERV_COMPACTION_SERVICE_ROOT_EXECUTORS(
"tserver.compaction.major.service.root.planner.opts.executors",
- "[{'name':'small','maxSize':'32M','numThreads':1},"
+ "[{'name':'small','maxSize':'32M','numThreads':1},".replaceAll("'", "\"")
+ "{'name':'huge','numThreads':1}]".replaceAll("'", "\""),
Review comment:
Could do this instead:
```suggestion
("[{'name':'small','maxSize':'32M','numThreads':1},"
+ "{'name':'huge','numThreads':1}]").replaceAll("'", "\""),
```
--
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] [accumulo] milleruntime commented on a change in pull request #2109: Apply replaceAll to all parts of JSON strings
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2109:
URL: https://github.com/apache/accumulo/pull/2109#discussion_r632034255
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -442,7 +442,7 @@
"The maximum number of files a compaction will open"),
TSERV_COMPACTION_SERVICE_ROOT_EXECUTORS(
"tserver.compaction.major.service.root.planner.opts.executors",
- "[{'name':'small','maxSize':'32M','numThreads':1},"
+ "[{'name':'small','maxSize':'32M','numThreads':1},".replaceAll("'", "\"")
+ "{'name':'huge','numThreads':1}]".replaceAll("'", "\""),
Review comment:
Oh yeah good idea.
--
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