[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/5872


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[NicoK]

Github user NicoK commented on a diff in the pull request:

https://github.com/apache/flink/pull/5872#discussion_r182478661
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ---
@@ -857,7 +859,7 @@ public void run() {
// the 'min delay between checkpoints'
lastCheckpointCompletionNanos = System.nanoTime();
 
-   LOG.info("Completed checkpoint {} ({} bytes in {} ms).", 
checkpointId,
+   LOG.info("Completed checkpoint {} for job {} ({} bytes in {} 
ms).", checkpointId, job,
--- End diff --

I would say, that "checkpoint x for job y" has these properties, not 
"checkpoint x" alone


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[zentol]

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/5872#discussion_r182461066
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ---
@@ -857,7 +859,7 @@ public void run() {
// the 'min delay between checkpoints'
lastCheckpointCompletionNanos = System.nanoTime();
 
-   LOG.info("Completed checkpoint {} ({} bytes in {} ms).", 
checkpointId,
+   LOG.info("Completed checkpoint {} for job {} ({} bytes in {} 
ms).", checkpointId, job,
--- End diff --

Hehe, i was going to say that it makes more sense to group checkpoint 
information (id and size) together ;)

Won't block the PR on this though.


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[zentol]

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/5872#discussion_r182459949
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ---
@@ -1214,7 +1216,7 @@ public void run() {
triggerCheckpoint(System.currentTimeMillis(), 
true);
}
catch (Exception e) {
-   LOG.error("Exception while triggering 
checkpoint.", e);
+   LOG.error("Exception while triggering 
checkpoint for job " + job + ".", e);
--- End diff --

Let's use this opportunity to switch for formats for the changed lines.


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[NicoK]

Github user NicoK commented on a diff in the pull request:

https://github.com/apache/flink/pull/5872#discussion_r182455513
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ---
@@ -857,7 +859,7 @@ public void run() {
// the 'min delay between checkpoints'
lastCheckpointCompletionNanos = System.nanoTime();
 
-   LOG.info("Completed checkpoint {} ({} bytes in {} ms).", 
checkpointId,
+   LOG.info("Completed checkpoint {} for job {} ({} bytes in {} 
ms).", checkpointId, job,
--- End diff --

I actually prefer if before the brackets - it makes a bit more sense there


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[NicoK]

Github user NicoK commented on a diff in the pull request:

https://github.com/apache/flink/pull/5872#discussion_r182455202
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ---
@@ -1214,7 +1216,7 @@ public void run() {
triggerCheckpoint(System.currentTimeMillis(), 
true);
}
catch (Exception e) {
-   LOG.error("Exception while triggering 
checkpoint.", e);
+   LOG.error("Exception while triggering 
checkpoint for job " + job + ".", e);
--- End diff --

I was also thinking about this but decided to keep the style of each 
individual message (ok, there was no previous style here). I could change all 
changed lines to adhere the string formats instead of string concatenation if 
you think, that's better


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[zentol]

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/5872#discussion_r182441934
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ---
@@ -1214,7 +1216,7 @@ public void run() {
triggerCheckpoint(System.currentTimeMillis(), 
true);
}
catch (Exception e) {
-   LOG.error("Exception while triggering 
checkpoint.", e);
+   LOG.error("Exception while triggering 
checkpoint for job " + job + ".", e);
--- End diff --

use `{}` instead? (also applies to other changed lines)


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[zentol]

Github user zentol commented on a diff in the pull request:

https://github.com/apache/flink/pull/5872#discussion_r182442219
  
--- Diff: 
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
 ---
@@ -857,7 +859,7 @@ public void run() {
// the 'min delay between checkpoints'
lastCheckpointCompletionNanos = System.nanoTime();
 
-   LOG.info("Completed checkpoint {} ({} bytes in {} ms).", 
checkpointId,
+   LOG.info("Completed checkpoint {} for job {} ({} bytes in {} 
ms).", checkpointId, job,
--- End diff --

maybe move job to the end of the message, i.e. `Completed checkpoint {} ({} 
bytes in {} ms) for job {} .`


---

[GitHub] flink pull request #5872: [FLINK-9206][checkpoints] add job IDs to Checkpoin...

[NicoK]

GitHub user NicoK opened a pull request:

https://github.com/apache/flink/pull/5872

[FLINK-9206][checkpoints] add job IDs to CheckpointCoordinator log messages

## What is the purpose of the change

The `CheckpointCoordinator` exists per job but its log messages (all 
bundled inside a single JobManager log) do not always contain this job ID 
making it hard to distinguish different instances when running multiple jobs.

## Brief change log

- extend log messages with job information

## Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

## Does this pull request potentially affect one of the following parts:

  - Dependencies (does it add or upgrade a dependency): **no**
  - The public API, i.e., is any changed class annotated with 
`@Public(Evolving)`: **no**
  - The serializers: **no**
  - The runtime per-record code paths (performance sensitive): **no**
  - Anything that affects deployment or recovery: JobManager (and its 
components), Checkpointing, Yarn/Mesos, ZooKeeper: **yes**
  - The S3 file system connector: **no**

## Documentation

  - Does this pull request introduce a new feature? **no**
  - If yes, how is the feature documented? **not applicable**


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/NicoK/flink flink-9206

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/5872.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #5872


commit d57ce2a4094d32b7aa0a8380bd44e4f3a80f8062
Author: Nico Kruber 
Date:   2018-04-18T13:01:32Z

[FLINK-9206][checkpoints] add job IDs to CheckpointCoordinator log messages




---