Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-05-08 Thread via GitHub 


zhuzhurk merged PR #24747:
URL: https://github.com/apache/flink/pull/24747


-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-05-08 Thread via GitHub 


HCTommy commented on PR #24747:
URL: https://github.com/apache/flink/pull/24747#issuecomment-2100853984

   Hi @zhuzhurk  , kooking for your review again. Tkanks


-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-05-07 Thread via GitHub 


zhuzhurk commented on code in PR #24747:
URL: https://github.com/apache/flink/pull/24747#discussion_r1593457699


##
flink-filesystems/flink-oss-fs-hadoop/src/main/java/org/apache/flink/fs/osshadoop/writer/OSSCommitter.java:
##
@@ -75,8 +75,7 @@ public void commitAfterRecovery() throws IOException {
 ossAccessor.completeMultipartUpload(objectName, uploadId, 
partETags);
 } catch (Exception e) {
 LOG.info(
-"Failed to commit after recovery {} with multi-part 
upload ID {}. "
-+ "exception {}",
+"Failed to commit after recovery {} with multi-part 
upload ID {}. ",

Review Comment:
   Better to remove the trailing space.



##
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java:
##
@@ -1427,7 +1427,9 @@ private static String 
encodeYarnLocalResourceDescriptorListToString(
  */
 private void failSessionDuringDeployment(
 YarnClient yarnClient, YarnClientApplication yarnApplication) {
-LOG.info("Killing YARN application");
+LOG.info(

Review Comment:
   Se my other comment around. I think this enrichment is not necessary.



##
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java:
##
@@ -1792,6 +1794,8 @@ public void run() {
 if (!fs.delete(yarnFilesDir, true)) {
 throw new IOException(
 "Deleting files in " + yarnFilesDir + " was 
unsuccessful");
+} else {
+LOG.info("Deleting files in {} was successful", 
yarnFilesDir);

Review Comment:
   There is no need to log such an info log if no error happens.



##
flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobClient.java:
##
@@ -130,7 +130,12 @@ static void downloadFromBlobServer(
 throws IOException {
 
 final byte[] buf = new byte[BUFFER_SIZE];
-LOG.info("Downloading {}/{} from {}", jobId, blobKey, serverAddress);
+LOG.info(
+"Downloading {}/{} from {} (retry {})",

Review Comment:
   I think the `retry` is not needed here. If a retry happens, the retry 
attempt is already logged (in the code below).
   Besides that, `numFetchRetries` is not the current retry attempt but is the 
max retry count.



-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-05-06 Thread via GitHub 


zhuzhurk commented on code in PR #24747:
URL: https://github.com/apache/flink/pull/24747#discussion_r1590940937


##
flink-clients/src/main/java/org/apache/flink/client/deployment/application/executors/EmbeddedExecutor.java:
##
@@ -163,7 +163,7 @@ private static CompletableFuture submitJob(
 final Time rpcTimeout) {
 checkNotNull(jobGraph);
 
-LOG.info("Submitting Job with JobId={}.", jobGraph.getJobID());
+LOG.info("Submitting job '{}' ({}).", jobGraph.getName(), 
jobGraph.getJobID());

Review Comment:
   The job submission log of application mode is already a bit redundant. 
Therefore I prefer to not further complicate it. 
   
   e.g.
   ```
   2024-05-06 20:19:20,853 [flink-akka.actor.default-dispatcher-16] INFO  
org.apache.flink.client.deployment.application.executors.EmbeddedExecutor [] - 
Job 2214f37f6f124fadb641e22182325897 is submitted.
   2024-05-06 20:19:20,854 [flink-akka.actor.default-dispatcher-16] INFO  
org.apache.flink.client.deployment.application.executors.EmbeddedExecutor [] - 
Submitting Job with JobId=2214f37f6f124fadb641e22182325897.
   2024-05-06 20:19:21,141 [flink-akka.actor.default-dispatcher-5] INFO  
org.apache.flink.runtime.dispatcher.StandaloneDispatcher [] - Received 
JobGraph submission 'Flink Streaming Job' (2214f37f6f124fadb641e22182325897).
   2024-05-06 20:19:21,177 [flink-akka.actor.default-dispatcher-5] INFO  
org.apache.flink.runtime.dispatcher.StandaloneDispatcher [] - Submitting 
job 'Flink Streaming Job' (2214f37f6f124fadb641e22182325897).
   ```
   



-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-05-06 Thread via GitHub 


zhuzhurk commented on code in PR #24747:
URL: https://github.com/apache/flink/pull/24747#discussion_r1590908511


##
flink-filesystems/flink-oss-fs-hadoop/src/main/java/org/apache/flink/fs/osshadoop/writer/OSSCommitter.java:
##
@@ -76,7 +76,7 @@ public void commitAfterRecovery() throws IOException {
 } catch (Exception e) {
 LOG.info(
 "Failed to commit after recovery {} with multi-part 
upload ID {}. "
-+ "exception {}",
++ "exception",

Review Comment:
   Maybe just removing "exception..."



##
flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/io/checkpointing/BarrierAlignmentUtilTest.java:
##
@@ -63,7 +63,7 @@ void testDelayableTimerNotHiddenException() throws Exception {
 timerService.advance(delay.toMillis());
 
 assertThatThrownBy(mailboxProcessor::runMailboxStep)
-.as("BarrierAlignmentUtil.DelayableTimer should not hidden 
exception")
+.as("BarrierAlignmentUtil.DelayableTimer should not hide 
exception")

Review Comment:
   exception -> exceptions



##
flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobClient.java:
##
@@ -130,7 +130,13 @@ static void downloadFromBlobServer(
 throws IOException {
 
 final byte[] buf = new byte[BUFFER_SIZE];
-LOG.info("Downloading {}/{} from {}", jobId, blobKey, serverAddress);
+LOG.info(
+"Downloading {}/{} from {} and store it under {} (retry {})",
+jobId,
+blobKey,
+serverAddress,
+localJarFile.getAbsolutePath(),

Review Comment:
   My gut feeling is that this path is a bit too detailed to be in `INFO` logs.
   Would you elaborate more on the scenarios that you find it is essential to 
log it?



##
flink-clients/src/main/java/org/apache/flink/client/deployment/application/executors/EmbeddedExecutor.java:
##
@@ -163,7 +163,7 @@ private static CompletableFuture submitJob(
 final Time rpcTimeout) {
 checkNotNull(jobGraph);
 
-LOG.info("Submitting Job with JobId={}.", jobGraph.getJobID());
+LOG.info("Submitting job '{}' ({}).", jobGraph.getName(), 
jobGraph.getJobID());

Review Comment:
   The job submission log of application mode is already redundant. Therefore I 
prefer to not further complicate it. 
   
   e.g.
   ```
   2024-05-06 20:19:20,853 [flink-akka.actor.default-dispatcher-16] INFO  
org.apache.flink.client.deployment.application.executors.EmbeddedExecutor [] - 
Job 2214f37f6f124fadb641e22182325897 is submitted.
   2024-05-06 20:19:20,854 [flink-akka.actor.default-dispatcher-16] INFO  
org.apache.flink.client.deployment.application.executors.EmbeddedExecutor [] - 
Submitting Job with JobId=2214f37f6f124fadb641e22182325897.
   2024-05-06 20:19:21,141 [flink-akka.actor.default-dispatcher-5] INFO  
org.apache.flink.runtime.dispatcher.StandaloneDispatcher [] - Received 
JobGraph submission 'Flink Streaming Job' (2214f37f6f124fadb641e22182325897).
   2024-05-06 20:19:21,177 [flink-akka.actor.default-dispatcher-5] INFO  
org.apache.flink.runtime.dispatcher.StandaloneDispatcher [] - Submitting 
job 'Flink Streaming Job' (2214f37f6f124fadb641e22182325897).
   ```
   



##
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java:
##
@@ -1309,10 +1309,10 @@ private ApplicationReport startAppMaster(
 + appId);
 // break ..
 case RUNNING:
-LOG.info("YARN application has been deployed 
successfully.");
+LOG.info("YARN application {} has been deployed 
successfully.", appId);

Review Comment:
   In my experiences using yarn + flink, this enrichment is not needed because 
the application id is just around.
   So I think these changes to `YarnClusterDescriptor` do not seem to be 
necessary and can be omitted.



-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-05-05 Thread via GitHub 


HCTommy commented on PR #24747:
URL: https://github.com/apache/flink/pull/24747#issuecomment-2094854809

   Hi, @fredia . I optimized some logs, which I think is helpful for people to 
debug in production environment. Could you please review in your available 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.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-04-29 Thread via GitHub 


flinkbot commented on PR #24747:
URL: https://github.com/apache/flink/pull/24747#issuecomment-2083291280

   
   ## CI report:
   
   * 9a53729a7c3d968977bcbae010246a28269f9359 UNKNOWN
   
   
   Bot commands
 The @flinkbot bot supports the following commands:
   
- `@flinkbot run azure` re-run the last Azure build
   


-- 
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-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-35270]Enrich information in logs, making it easier for debugging [flink]

2024-04-29 Thread via GitHub 


HCTommy commented on PR #24747:
URL: https://github.com/apache/flink/pull/24747#issuecomment-2083286683

   Hi @zhuzhurk , I optimized some logs, which I think is helpful for people to 
debug in production environment. Could you please review in your available 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.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org