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 2020/02/07 14:20:41 UTC
[GitHub] [flink-statefun] tzulitai opened a new pull request #16:
[FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
tzulitai opened a new pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16
Prior to this PR, Stateful Functions allow enabling or disabling multiplex state, regardless of the state backend being used. This actually does not make sense, considering that:
* If the heap backend is used, there is no reason to multiplex state. Also taking into account that there are problems that exist for multiplexing state such as lack of support for state schema evolution.
* If RocksDB backend is used, it is highly encouraged to multiplex state, anyways.
Therefore, this PR removes the `MULTIPLEX_FLINK_STATE` configuration and simply piggybacks the behaviour of multiplexing state to the state backend being used, i.e. multiplex only when RocksDB state backend is used.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] igalshilman commented on a change in pull request
#16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
igalshilman commented on a change in pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16#discussion_r376499213
##########
File path: statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/functions/Reductions.java
##########
@@ -145,4 +142,21 @@ void processEnvelopes() {
// TODO: consider preemption if too many local messages.
}
}
+
+ private static boolean useMultiplexedState(KeyedStateBackend<?> keyedStateBackend) {
+ final String backendClassName = keyedStateBackend.getClass().getName();
+
+ // TODO this is fragile and error-prone to classname changes, but we're doing this
+ // TODO to avoid additional dependencies on the Flink state backends
+ // TODO ideally, we should revisit how configuration is being passed to the
+ // TODO operators to be available at runtime
+ if (backendClassName.equals("org.apache.flink.runtime.state.heap.HeapKeyedStateBackend")) {
+ return false;
+ }
+ if (backendClassName.equals(
+ "org.apache.flink.contrib.streaming.state.RocksDBKeyedStateBackend")) {
Review comment:
Ok, I actually see that the build fails due to this :-)
I'd vote to only focus on RocksDB.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16#discussion_r376677296
##########
File path: statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/functions/Reductions.java
##########
@@ -145,4 +142,21 @@ void processEnvelopes() {
// TODO: consider preemption if too many local messages.
}
}
+
+ private static boolean useMultiplexedState(KeyedStateBackend<?> keyedStateBackend) {
+ final String backendClassName = keyedStateBackend.getClass().getName();
+
+ // TODO this is fragile and error-prone to classname changes, but we're doing this
+ // TODO to avoid additional dependencies on the Flink state backends
+ // TODO ideally, we should revisit how configuration is being passed to the
+ // TODO operators to be available at runtime
+ if (backendClassName.equals("org.apache.flink.runtime.state.heap.HeapKeyedStateBackend")) {
+ return false;
+ }
+ if (backendClassName.equals(
+ "org.apache.flink.contrib.streaming.state.RocksDBKeyedStateBackend")) {
Review comment:
@igalshilman 👍 Makes total sense. Will simply this branch.
^ the state backend is never `null` up to this point.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai closed pull request #16: [FLINK-15945]
Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
tzulitai closed pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on issue #16: [FLINK-15945]
Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
tzulitai commented on issue #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16#issuecomment-583688306
I've updated to simplify the branches.
Will merge this once Travis agrees.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] tzulitai commented on a change in pull request
#16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
tzulitai commented on a change in pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16#discussion_r376677296
##########
File path: statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/functions/Reductions.java
##########
@@ -145,4 +142,21 @@ void processEnvelopes() {
// TODO: consider preemption if too many local messages.
}
}
+
+ private static boolean useMultiplexedState(KeyedStateBackend<?> keyedStateBackend) {
+ final String backendClassName = keyedStateBackend.getClass().getName();
+
+ // TODO this is fragile and error-prone to classname changes, but we're doing this
+ // TODO to avoid additional dependencies on the Flink state backends
+ // TODO ideally, we should revisit how configuration is being passed to the
+ // TODO operators to be available at runtime
+ if (backendClassName.equals("org.apache.flink.runtime.state.heap.HeapKeyedStateBackend")) {
+ return false;
+ }
+ if (backendClassName.equals(
+ "org.apache.flink.contrib.streaming.state.RocksDBKeyedStateBackend")) {
Review comment:
@igalshilman 👍 Makes total sense. Will simplify this branch.
^ the state backend is never `null` up to this point.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] igalshilman commented on a change in pull request
#16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
igalshilman commented on a change in pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16#discussion_r376418865
##########
File path: statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/functions/Reductions.java
##########
@@ -145,4 +142,21 @@ void processEnvelopes() {
// TODO: consider preemption if too many local messages.
}
}
+
+ private static boolean useMultiplexedState(KeyedStateBackend<?> keyedStateBackend) {
+ final String backendClassName = keyedStateBackend.getClass().getName();
+
+ // TODO this is fragile and error-prone to classname changes, but we're doing this
+ // TODO to avoid additional dependencies on the Flink state backends
+ // TODO ideally, we should revisit how configuration is being passed to the
+ // TODO operators to be available at runtime
+ if (backendClassName.equals("org.apache.flink.runtime.state.heap.HeapKeyedStateBackend")) {
+ return false;
+ }
+ if (backendClassName.equals(
+ "org.apache.flink.contrib.streaming.state.RocksDBKeyedStateBackend")) {
Review comment:
@tzulitai I think that we only need this branch,
and this method can be simplified to simply:
`return backendClassName.equals(
"org.apache.flink.contrib.streaming.state.RocksDBKeyedStateBackend")`
I think that we shouldn't care about any other state backends.
In addition, can a state backend be NULL, or is there a default toy state backend that is used for the mini cluster or something a long these lines?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] sjwiesman commented on a change in pull request
#16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
sjwiesman commented on a change in pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16#discussion_r376502933
##########
File path: statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/functions/Reductions.java
##########
@@ -145,4 +142,21 @@ void processEnvelopes() {
// TODO: consider preemption if too many local messages.
}
}
+
+ private static boolean useMultiplexedState(KeyedStateBackend<?> keyedStateBackend) {
+ final String backendClassName = keyedStateBackend.getClass().getName();
+
+ // TODO this is fragile and error-prone to classname changes, but we're doing this
+ // TODO to avoid additional dependencies on the Flink state backends
+ // TODO ideally, we should revisit how configuration is being passed to the
+ // TODO operators to be available at runtime
+ if (backendClassName.equals("org.apache.flink.runtime.state.heap.HeapKeyedStateBackend")) {
+ return false;
+ }
+ if (backendClassName.equals(
+ "org.apache.flink.contrib.streaming.state.RocksDBKeyedStateBackend")) {
Review comment:
@igalshilman, the statebackend cannot be null. The default is MemoryStateBackend which is the same implementation as FsStateBackend but checks are stored in JM memory.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [flink-statefun] igalshilman commented on a change in pull request
#16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
Posted by GitBox <gi...@apache.org>.
igalshilman commented on a change in pull request #16: [FLINK-15945] Remove MULTIPLEX_FLINK_STATE config
URL: https://github.com/apache/flink-statefun/pull/16#discussion_r376419898
##########
File path: statefun-flink/statefun-flink-core/src/main/java/org/apache/flink/statefun/flink/core/functions/Reductions.java
##########
@@ -79,7 +76,7 @@ static Reductions create(
container.add("keyed-state-backend", KeyedStateBackend.class, keyedStateBackend);
container.add(new DynamicallyRegisteredTypes(statefulFunctionsUniverse.types()));
- if (configuration.getBoolean(StatefulFunctionsJobConstants.MULTIPLEX_FLINK_STATE)) {
+ if (useMultiplexedState(keyedStateBackend)) {
Review comment:
👍
----------------------------------------------------------------
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
With regards,
Apache Git Services