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 2022/08/15 17:34:50 UTC
[GitHub] [flink] rkhachatryan opened a new pull request, #20585: [FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
rkhachatryan opened a new pull request, #20585:
URL: https://github.com/apache/flink/pull/20585
## What is the purpose of the change
Spread changelog materialization and consequently deletion, more evenly in time.
## Verifying this change
TBD
## 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, Kubernetes/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? no
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] fredia commented on a diff in pull request #20585: [FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
fredia commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r955709811
##########
flink-state-backends/flink-statebackend-common/src/test/java/org/apache/flink/state/common/PeriodicMaterializationManagerTest.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.state.common;
+
+import org.apache.flink.runtime.concurrent.ManuallyTriggeredScheduledExecutorService;
+import org.apache.flink.runtime.mailbox.SyncMailboxExecutor;
+import org.apache.flink.runtime.metrics.groups.UnregisteredMetricGroups;
+import org.apache.flink.state.common.PeriodicMaterializationManager.MaterializationTarget;
+
+import org.junit.Test;
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static org.apache.flink.shaded.guava30.com.google.common.collect.Iterators.getOnlyElement;
+import static org.apache.flink.util.concurrent.Executors.newDirectExecutorService;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.lessThanOrEqualTo;
+
+/** {@link PeriodicMaterializationManager} test. */
+public class PeriodicMaterializationManagerTest {
Review Comment:
nit: extends TestLogger?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] masteryhx commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
masteryhx commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946405835
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
I think disabling materialization for test is not the main purpose of this pr, right ?
(Diabling has been supported in [FLINK-26673](https://issues.apache.org/jira/browse/FLINK-26673))
About making the checkpoint deletion evenly, It is not enough for users to using changlog out of box without configuring other configs like [jobmanager.io-pool.size](https://nightlies.apache.org/flink/flink-docs-master/docs/deployment/config/#jobmanager-io-pool-size) ?
We may still need other strategy to solve the problem of the checkpoint deletion delay ?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #20585: [FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #20585:
URL: https://github.com/apache/flink/pull/20585#issuecomment-1215486750
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "a7403d125359e904f3665969a3ccc642223ae547",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "a7403d125359e904f3665969a3ccc642223ae547",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* a7403d125359e904f3665969a3ccc642223ae547 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] curcur commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
curcur commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946444390
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
So now we have `periodicMaterializationDelay`, we have `initialDelay` and we have `periodicMaterializationDelay < 0` to indicate materialization disabled if I understand correctly?
Isn't this so confusing and misleading to the users? What about `initialDelay` being very big or negative?
What is the problem to introduce a flag to indicate enable/disable materialization ? I think the original argument is that we do not want to introduce too many configs, but I think the current way is much more confusing and problematic.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] fredia commented on pull request #20585: [FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
fredia commented on PR #20585:
URL: https://github.com/apache/flink/pull/20585#issuecomment-1228135936
Thanks for creating this PR @rkhachatryan, LGTM.
I noticed you mentioned this PR under [FLINK-26590](https://issues.apache.org/jira/browse/FLINK-26590?focusedCommentId=17579828&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17579828) :
> I found that the problem is caused by [FLINK-28976](https://issues.apache.org/jira/browse/FLINK-28976) in some setups.
The fix for which is much more simple, so I'd prioritize it over this ticket.
Why **removing `materialization_interval`** can solve FLINK-26590?
I guess “removing `materialization_interval`” makes the size of 1st materialization smaller, thus the deletion of shared state can be faster?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] curcur commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
curcur commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946315050
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
Let's introduce a way to disable the materialization (flag).
I am not a fan of using `periodicMaterializationDelay` very long to disable materialization. It is implicit and may lead to error.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] fredia commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
fredia commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946284865
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
> So we might want to (re)introduce a flag to disable materialization.
Currently, if `PERIODIC_MATERIALIZATION_INTERVAL` is negative, the periodic materialization will be disabled. Do you mean to introduce a **new** flag?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] curcur commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
curcur commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946315050
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
I think we should have some way to disable the materialization (flag).
I am not a fan of using `periodicMaterializationDelay` very long to disable materialization. It is implicit and may lead error.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] curcur commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
curcur commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946444390
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
So now we have `periodicMaterializationDelay`, we have `initialDelay` and we have `periodicMaterializationDelay < 0` to indicate materialization disabled if I understand correctly?
Isn't this so confusing and misleading to the users? What about `initialDelay` being very long or negative?
What is the problem to introduce a flag to indicate enable/disable? I think the original argument is that we do not want to introduce too many configs, but I think the current way is much more confusing and problematic.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] curcur commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
curcur commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946444390
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
So now we have `periodicMaterializationDelay`, we have `initialDelay` and we have `periodicMaterializationDelay < 0` to indicate materialization disabled if I understand correctly?
Isn't this so confusing and misleading to the users? What about `initialDelay` being very big or negative?
What is the problem to introduce a flag to indicate enable/disable? I think the original argument is that we do not want to introduce too many configs, but I think the current way is much more confusing and problematic.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on pull request #20585: [FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on PR #20585:
URL: https://github.com/apache/flink/pull/20585#issuecomment-1228166200
Thanks for the review @fredia,
> Why removing materialization_interval can solve [FLINK-26590](https://issues.apache.org/jira/browse/FLINK-26590)?
> I guess “removing materialization_interval” makes the size of 1st materialization smaller, thus the deletion of shared state can be faster?
Yes, exactly.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan merged pull request #20585: [FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
rkhachatryan merged PR #20585:
URL: https://github.com/apache/flink/pull/20585
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20585: [FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r945980471
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
This change affects the tests which use very high `periodicMaterializationDelay` to effictively disable materialization. There, `periodicMaterializationDelay` set to `Long.MAX` may or may not disable the materialization.
So we might want to (re)introduce a flag to disable materialization.
WDYT @curcur ?
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] rkhachatryan commented on a diff in pull request #20585: [WIP][FLINK-28976][state/changelog] Don't add extra delay to the 1st materialization
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on code in PR #20585:
URL: https://github.com/apache/flink/pull/20585#discussion_r946514885
##########
flink-state-backends/flink-statebackend-changelog/src/main/java/org/apache/flink/state/changelog/PeriodicMaterializationManager.java:
##########
@@ -118,7 +118,7 @@ public void start() {
LOG.info("Task {} starts periodic materialization", subtaskName);
- scheduleNextMaterialization(periodicMaterializeDelay + initialDelay);
+ scheduleNextMaterialization(initialDelay);
Review Comment:
Sorry for the confusion, a negative value will prevent any materialization indeed.
I think both approaches have their both pros and cons (a separate flag is more explicit, but the user has to keep the two variables consistent).
But keeping negative value as it is doesn't change the API, so I'd chose this option.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org