You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2021/02/26 08:35:55 UTC
[GitHub] [flink] zentol opened a new pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
zentol opened a new pull request #15030:
URL: https://github.com/apache/flink/pull/15030
Adds the `numRestarts` metric to the adaptive scheduler. It effectively counts the number of times we have transitioned into the `Restarting` state.
----------------------------------------------------------------
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] [flink] zentol commented on a change in pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
zentol commented on a change in pull request #15030:
URL: https://github.com/apache/flink/pull/15030#discussion_r584617385
##########
File path: flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/core/testutils/ManuallyTriggeredScheduledExecutorService.java
##########
@@ -166,6 +166,19 @@ public boolean awaitTermination(long timeout, TimeUnit unit) {
// Execution triggering and access to the queued tasks
// ------------------------------------------------------------------------
+ /**
+ * Executes all runnable and scheduled non-periodic tasks until none are left to run. This is
+ * essentially a combination of {@link #triggerAll()} and {@link
+ * #triggerNonPeriodicScheduledTasks()} that allows making a test agnostic of how exactly a
+ * runnable is passed to the executor.
+ */
+ public void triggerAllNonPeriodicTasks() {
Review comment:
This method only exists to cover both runnable and non-periodic scheduled tasks, for the simple reason that I always had to check "is the code using execute() or schedule()?" and that became rather error-prone and annoying very quickly.
----------------------------------------------------------------
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] [flink] XComp commented on a change in pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
XComp commented on a change in pull request #15030:
URL: https://github.com/apache/flink/pull/15030#discussion_r584608406
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/adaptive/AdaptiveSchedulerTest.java
##########
@@ -290,6 +303,85 @@ public void testResourceTimeout() throws Exception {
assertThat(b, is(true));
}
+ @Test
+ public void testNumRestartsMetric() throws Exception {
+ final CompletableFuture<Gauge<Integer>> numRestartsMetricFuture = new CompletableFuture<>();
+ final MetricRegistry metricRegistry =
+ TestingMetricRegistry.builder()
+ .setRegisterConsumer(
+ (metric, name, group) -> {
+ if (MetricNames.NUM_RESTARTS.equals(name)) {
+ numRestartsMetricFuture.complete((Gauge<Integer>) metric);
+ }
+ })
+ .build();
+
+ final JobGraph jobGraph = createJobGraph();
+
+ final DefaultDeclarativeSlotPool declarativeSlotPool =
+ new DefaultDeclarativeSlotPool(
+ jobGraph.getJobID(),
+ new DefaultAllocatedSlotPool(),
+ ignored -> {},
+ Time.minutes(10),
+ Time.minutes(10));
+
+ final Configuration configuration = new Configuration();
+ configuration.set(JobManagerOptions.MIN_PARALLELISM_INCREASE, 1);
+
+ final AdaptiveScheduler scheduler =
+ new AdaptiveSchedulerBuilder(jobGraph, mainThreadExecutor)
+ .setJobMasterConfiguration(configuration)
+ .setJobManagerJobMetricGroup(
+ new JobManagerJobMetricGroup(
+ metricRegistry,
+ createUnregisteredJobManagerMetricGroup(),
+ new JobID(),
+ "jobName"))
+ .setDeclarativeSlotPool(declarativeSlotPool)
+ .build();
+
+ final Gauge<Integer> numRestartsMetric = numRestartsMetricFuture.get();
+ assertThat(numRestartsMetric.getValue(), is(0));
Review comment:
I'm wondering whether we should move this assert even further down just before the new slot offering triggers a restart. That would make clearer which operation is expected to trigger the metric change.
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/scheduler/adaptive/AdaptiveSchedulerTest.java
##########
@@ -290,6 +303,85 @@ public void testResourceTimeout() throws Exception {
assertThat(b, is(true));
}
+ @Test
+ public void testNumRestartsMetric() throws Exception {
+ final CompletableFuture<Gauge<Integer>> numRestartsMetricFuture = new CompletableFuture<>();
+ final MetricRegistry metricRegistry =
+ TestingMetricRegistry.builder()
+ .setRegisterConsumer(
+ (metric, name, group) -> {
+ if (MetricNames.NUM_RESTARTS.equals(name)) {
+ numRestartsMetricFuture.complete((Gauge<Integer>) metric);
+ }
+ })
+ .build();
+
+ final JobGraph jobGraph = createJobGraph();
+
+ final DefaultDeclarativeSlotPool declarativeSlotPool =
+ new DefaultDeclarativeSlotPool(
+ jobGraph.getJobID(),
+ new DefaultAllocatedSlotPool(),
+ ignored -> {},
+ Time.minutes(10),
+ Time.minutes(10));
+
+ final Configuration configuration = new Configuration();
+ configuration.set(JobManagerOptions.MIN_PARALLELISM_INCREASE, 1);
+
+ final AdaptiveScheduler scheduler =
+ new AdaptiveSchedulerBuilder(jobGraph, mainThreadExecutor)
+ .setJobMasterConfiguration(configuration)
+ .setJobManagerJobMetricGroup(
+ new JobManagerJobMetricGroup(
+ metricRegistry,
+ createUnregisteredJobManagerMetricGroup(),
+ new JobID(),
+ "jobName"))
+ .setDeclarativeSlotPool(declarativeSlotPool)
+ .build();
+
+ final Gauge<Integer> numRestartsMetric = numRestartsMetricFuture.get();
+ assertThat(numRestartsMetric.getValue(), is(0));
+
+ scheduler.startScheduling();
+
+ final TaskExecutorGateway taskManagerGateway =
+ new TestingTaskExecutorGatewayBuilder()
+ .setCancelTaskFunction(
+ executionAttemptId -> {
+ mainThreadExecutor.execute(
+ () ->
+ scheduler.updateTaskExecutionState(
+ new TaskExecutionState(
+ executionAttemptId,
+ ExecutionState.CANCELED)));
+ return CompletableFuture.completedFuture(Acknowledge.get());
+ })
+ .createTestingTaskExecutorGateway();
+
+ declarativeSlotPool.offerSlots(
+ createSlotOffersForResourceRequirements(
+ ResourceCounter.withResource(ResourceProfile.UNKNOWN, 1)),
+ new LocalTaskManagerLocation(),
+ new RpcTaskManagerGateway(taskManagerGateway, JobMasterId.generate()),
Review comment:
A minor thing: Is there a reason why we use `RpcTaskManagerGateway` instead of `SimpleAckingTaskManagerGateway` here? It wouldn't make a big difference but would reduce the number of instances created by this class...
##########
File path: flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/core/testutils/ManuallyTriggeredScheduledExecutorService.java
##########
@@ -166,6 +166,19 @@ public boolean awaitTermination(long timeout, TimeUnit unit) {
// Execution triggering and access to the queued tasks
// ------------------------------------------------------------------------
+ /**
+ * Executes all runnable and scheduled non-periodic tasks until none are left to run. This is
+ * essentially a combination of {@link #triggerAll()} and {@link
+ * #triggerNonPeriodicScheduledTasks()} that allows making a test agnostic of how exactly a
+ * runnable is passed to the executor.
+ */
+ public void triggerAllNonPeriodicTasks() {
Review comment:
Just to make me understand: The method is added to cover cases where a `Runnable` or non-periodic scheduled task is added while this method is already processed? Hence, it has benefits in tests where there is no explicit wait before triggering the execution of those tasks?
----------------------------------------------------------------
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] [flink] flinkbot edited a comment on pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #15030:
URL: https://github.com/apache/flink/pull/15030#issuecomment-786508119
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836",
"triggerID" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7a3ef726583660832c4ec8040658722d3f7e00b1 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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] [flink] flinkbot commented on pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #15030:
URL: https://github.com/apache/flink/pull/15030#issuecomment-786496767
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit 7a3ef726583660832c4ec8040658722d3f7e00b1 (Fri Feb 26 08:37:32 UTC 2021)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
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] [flink] flinkbot edited a comment on pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #15030:
URL: https://github.com/apache/flink/pull/15030#issuecomment-786508119
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"status" : "DELETED",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836",
"triggerID" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"triggerType" : "PUSH"
}, {
"hash" : "070296e85d7b15d4baf115208d9cf17224e4b003",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=14050",
"triggerID" : "070296e85d7b15d4baf115208d9cf17224e4b003",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 070296e85d7b15d4baf115208d9cf17224e4b003 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=14050)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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] [flink] flinkbot edited a comment on pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #15030:
URL: https://github.com/apache/flink/pull/15030#issuecomment-786508119
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836",
"triggerID" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"triggerType" : "PUSH"
}, {
"hash" : "070296e85d7b15d4baf115208d9cf17224e4b003",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "070296e85d7b15d4baf115208d9cf17224e4b003",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7a3ef726583660832c4ec8040658722d3f7e00b1 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836)
* 070296e85d7b15d4baf115208d9cf17224e4b003 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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] [flink] flinkbot edited a comment on pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #15030:
URL: https://github.com/apache/flink/pull/15030#issuecomment-786508119
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836",
"triggerID" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7a3ef726583660832c4ec8040658722d3f7e00b1 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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] [flink] zentol merged pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
zentol merged pull request #15030:
URL: https://github.com/apache/flink/pull/15030
----------------------------------------------------------------
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] [flink] flinkbot edited a comment on pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #15030:
URL: https://github.com/apache/flink/pull/15030#issuecomment-786508119
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836",
"triggerID" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"triggerType" : "PUSH"
}, {
"hash" : "070296e85d7b15d4baf115208d9cf17224e4b003",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=14050",
"triggerID" : "070296e85d7b15d4baf115208d9cf17224e4b003",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7a3ef726583660832c4ec8040658722d3f7e00b1 Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=13836)
* 070296e85d7b15d4baf115208d9cf17224e4b003 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=14050)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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] [flink] flinkbot commented on pull request #15030: [FLINK-21458][coordination] Add numRestarts metric
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #15030:
URL: https://github.com/apache/flink/pull/15030#issuecomment-786508119
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "7a3ef726583660832c4ec8040658722d3f7e00b1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 7a3ef726583660832c4ec8040658722d3f7e00b1 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
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