You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/06/28 04:16:38 UTC
[GitHub] [ozone] sky76093016 opened a new pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
sky76093016 opened a new pull request #2372:
URL: https://github.com/apache/ozone/pull/2372
## What changes were proposed in this pull request?
Eliminate expensive string creation in debug log messages(For example: toString()).
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5383
## How was this patch tested?
No test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on a change in pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on a change in pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#discussion_r659537291
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
##########
@@ -266,7 +266,8 @@ private void createCheckpoint(Path trashRoot, Date date) throws IOException {
while (true) {
try {
fs.rename(current, checkpoint);
- LOG.debug("Created trash checkpoint: " + checkpoint.toUri().getPath());
+ LOG.debug("Created trash checkpoint: {}",
+ checkpoint.toUri().getPath());
Review comment:
got 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#issuecomment-869397976
Thanks to @jojochuang ’s review, I neglected this small problem and corrected it immediately.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#issuecomment-869522237
Thanks again to @jojochuang ’s review, a bug about MockCRLStore.java has been posted to new jira[HDDS-5396](https://issues.apache.org/jira/browse/HDDS-5396)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#issuecomment-869341083
Please @jojochuang review this, 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on a change in pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
jojochuang commented on a change in pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#discussion_r659505828
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
##########
@@ -201,9 +201,9 @@ public void run() {
if (now >= end) {
Collection<FileStatus> trashRoots;
trashRoots = fs.getTrashRoots(true); // list all trash dirs
- LOG.debug("Trash root Size: " + trashRoots.size());
+ LOG.debug("Trash root Size: {}" + trashRoots.size());
for (FileStatus trashRoot : trashRoots) { // dump each trash
- LOG.debug("Trashroot:" + trashRoot.getPath().toString());
+ LOG.debug("Trashroot: {}" + trashRoot.getPath());
Review comment:
here too
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
##########
@@ -126,9 +126,9 @@ public boolean moveToTrash(Path path) throws IOException {
Path trashRoot = this.fs.getTrashRoot(path);
String key = path.toUri().getPath();
- LOG.debug("Key path to moveToTrash: "+ key);
+ LOG.debug("Key path to moveToTrash: {}"+ key);
Review comment:
this'll still result in an string concantenation.
So change it to:
`LOG.debug("Key path to moveToTrash: {}",key);`
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
##########
@@ -266,7 +266,8 @@ private void createCheckpoint(Path trashRoot, Date date) throws IOException {
while (true) {
try {
fs.rename(current, checkpoint);
- LOG.debug("Created trash checkpoint: " + checkpoint.toUri().getPath());
+ LOG.debug("Created trash checkpoint: {}"
+ + checkpoint.toUri().getPath());
Review comment:
here too.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
##########
@@ -201,9 +201,9 @@ public void run() {
if (now >= end) {
Collection<FileStatus> trashRoots;
trashRoots = fs.getTrashRoots(true); // list all trash dirs
- LOG.debug("Trash root Size: " + trashRoots.size());
+ LOG.debug("Trash root Size: {}" + trashRoots.size());
Review comment:
same here. replace + with ,
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
##########
@@ -126,9 +126,9 @@ public boolean moveToTrash(Path path) throws IOException {
Path trashRoot = this.fs.getTrashRoot(path);
String key = path.toUri().getPath();
- LOG.debug("Key path to moveToTrash: "+ key);
+ LOG.debug("Key path to moveToTrash: {}"+ key);
String trashRootKey = trashRoot.toUri().getPath();
- LOG.debug("TrashrootKey for moveToTrash: "+ trashRootKey);
+ LOG.debug("TrashrootKey for moveToTrash: {}"+ trashRootKey);
Review comment:
ditto
##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockOutputStream.java
##########
@@ -595,7 +595,7 @@ private void setIoException(Exception e) {
IOException exception = new IOException(EXCEPTION_MSG + e.toString(), e);
ioException.compareAndSet(null, exception);
} else {
- LOG.debug("Previous request had already failed with " + ioe.toString()
+ LOG.debug("Previous request had already failed with " + ioe
Review comment:
please use parameterized logging.
For example, LOG.debug("Previous request had already failed with {}", ioe)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sky76093016 commented on pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
sky76093016 commented on pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#issuecomment-869430088
Of course,I can fix these two.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
jojochuang commented on pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#issuecomment-869428726
Can you also fix these two as well?
https://github.com/apache/ozone/blob/2254abfa4d8ee3fda24c3d7215b0ddca61632fec/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMSecurityProtocolFailoverProxyProvider.java#L213
https://github.com/apache/ozone/blob/2254abfa4d8ee3fda24c3d7215b0ddca61632fec/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMBlockLocationFailoverProxyProvider.java#L189
given the complexity of the debug message, I suggest to wrap them with LOG.isDebugEnabled() too.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on a change in pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
jojochuang commented on a change in pull request #2372:
URL: https://github.com/apache/ozone/pull/2372#discussion_r659531240
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/update/server/MockCRLStore.java
##########
@@ -107,7 +107,7 @@ public BigInteger issueCert() throws Exception {
scmCertStore.getCrls(ImmutableList.of(crlId.get()));
if (crlInfos.isEmpty()) {
- log.debug("CRL[0]: {}", crlInfos.get(0).toString());
+ log.debug("CRL[0]: {}", crlInfos.get(0));
Review comment:
this statement doesn't make sense.
if crlInfos is empty, crlInfos.get(0) would throw IndexOutOfBoundsException
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashPolicyOzone.java
##########
@@ -266,7 +266,8 @@ private void createCheckpoint(Path trashRoot, Date date) throws IOException {
while (true) {
try {
fs.rename(current, checkpoint);
- LOG.debug("Created trash checkpoint: " + checkpoint.toUri().getPath());
+ LOG.debug("Created trash checkpoint: {}",
+ checkpoint.toUri().getPath());
Review comment:
this one is tricky, because URI.getPath() is not a O(1) operation. It constructs a String on the fly.
We need to wrap this debug message with if (LOG.isDebugEnabled()) {
##########
File path: hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/update/server/MockCRLStore.java
##########
@@ -107,7 +107,7 @@ public BigInteger issueCert() throws Exception {
scmCertStore.getCrls(ImmutableList.of(crlId.get()));
if (crlInfos.isEmpty()) {
- log.debug("CRL[0]: {}", crlInfos.get(0).toString());
+ log.debug("CRL[0]: {}", crlInfos.get(0));
Review comment:
@xiaoyuyao any idea what this was meant for?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang merged pull request #2372: HDDS-5383. Eliminate expensive string creation in debug log messages
Posted by GitBox <gi...@apache.org>.
jojochuang merged pull request #2372:
URL: https://github.com/apache/ozone/pull/2372
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org