[GitHub] [flink] Aitozi commented on a change in pull request #8479: [FLINK-11193][State Backends]Use user passed configuration overriding default configuration loading from file
Aitozi commented on a change in pull request #8479: [FLINK-11193][State
Backends]Use user passed configuration overriding default configuration loading
from file
URL: https://github.com/apache/flink/pull/8479#discussion_r321333537
##
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackend.java
##
@@ -314,10 +315,12 @@ private RocksDBStateBackend(RocksDBStateBackend
original, Configuration config,
this.enableTtlCompactionFilter =
original.enableTtlCompactionFilter
.resolveUndefined(config.getBoolean(TTL_COMPACT_FILTER_ENABLED));
- final String priorityQueueTypeString =
config.getString(TIMER_SERVICE_FACTORY);
-
- this.priorityQueueStateType = priorityQueueTypeString.length()
> 0 ?
-
PriorityQueueStateType.valueOf(priorityQueueTypeString.toUpperCase()) :
original.priorityQueueStateType;
+ if (original.priorityQueueStateType ==
PriorityQueueStateType.UNDEFINED) {
+ String priorityQueueTypeString =
config.getString(TIMER_SERVICE_FACTORY);
+ this.priorityQueueStateType =
PriorityQueueStateType.valueOf(priorityQueueTypeString.toUpperCase());
Review comment:
agree to use `null` directly.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Aitozi commented on a change in pull request #8479: [FLINK-11193][State Backends]Use user passed configuration overriding default configuration loading from file
Aitozi commented on a change in pull request #8479: [FLINK-11193][State
Backends]Use user passed configuration overriding default configuration loading
from file
URL: https://github.com/apache/flink/pull/8479#discussion_r289894942
##
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackend.java
##
@@ -295,6 +297,7 @@ public RocksDBStateBackend(AbstractStateBackend
checkpointStreamBackend, boolean
* @param classLoader The class loader.
*/
private RocksDBStateBackend(RocksDBStateBackend original, Configuration
config, ClassLoader classLoader) {
+ this.configuration = config;
Review comment:
I think we don't have to, I prefer to keep the former style, but only use
the config in user code with higher priority than the default configuration
loading from the file.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Aitozi commented on a change in pull request #8479: [FLINK-11193][State Backends]Use user passed configuration overriding default configuration loading from file
Aitozi commented on a change in pull request #8479: [FLINK-11193][State
Backends]Use user passed configuration overriding default configuration loading
from file
URL: https://github.com/apache/flink/pull/8479#discussion_r289677082
##
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackend.java
##
@@ -295,6 +297,7 @@ public RocksDBStateBackend(AbstractStateBackend
checkpointStreamBackend, boolean
* @param classLoader The class loader.
*/
private RocksDBStateBackend(RocksDBStateBackend original, Configuration
config, ClassLoader classLoader) {
+ this.configuration = config;
Review comment:
other config are all following this manner: check the original config
whether set or undefined, if it is not defined then it will look into the
config , at last fallback to the default config which is consistent with the
doc of the `config()`
> Creates a copy of this state backend that uses the values defined in the
configuration for fields where that **were not yet specified** in this state
backend.
But for the `priorityQueueStateType` there is no undefined type, it only has
two: *rocksdb* and *heap*, if we have to follow the manner I think we just have
to add an undefined value for `priorityQueueStateType`, but i am not sure do we
need to do this.
What i am do now is just overriding the config loading from file with the
original statebackend config to solve this problem.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Aitozi commented on a change in pull request #8479: [FLINK-11193][State Backends]Use user passed configuration overriding default configuration loading from file
Aitozi commented on a change in pull request #8479: [FLINK-11193][State
Backends]Use user passed configuration overriding default configuration loading
from file
URL: https://github.com/apache/flink/pull/8479#discussion_r289677082
##
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackend.java
##
@@ -295,6 +297,7 @@ public RocksDBStateBackend(AbstractStateBackend
checkpointStreamBackend, boolean
* @param classLoader The class loader.
*/
private RocksDBStateBackend(RocksDBStateBackend original, Configuration
config, ClassLoader classLoader) {
+ this.configuration = config;
Review comment:
other config are all follow this manner: check the original config whether
set or undefined, if it is not defined then it will look into the config , at
last fallback to the default config which is consistent with the doc of the
`config()`
> Creates a copy of this state backend that uses the values defined in the
configuration for fields where that **were not yet specified** in this state
backend.
But for the `priorityQueueStateType` there is no undefined type, it only has
two: *rocksdb* and *heap*, if we have to follow the manner I think we just have
to add an undefined value for `priorityQueueStateType`, but i am not sure do we
need to do this.
What i am do now is just overriding the config loading from file with the
original statebackend config to solve this problem.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Aitozi commented on a change in pull request #8479: [FLINK-11193][State Backends]Use user passed configuration overriding default configuration loading from file
Aitozi commented on a change in pull request #8479: [FLINK-11193][State
Backends]Use user passed configuration overriding default configuration loading
from file
URL: https://github.com/apache/flink/pull/8479#discussion_r286922969
##
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackend.java
##
@@ -295,6 +297,7 @@ public RocksDBStateBackend(AbstractStateBackend
checkpointStreamBackend, boolean
* @param classLoader The class loader.
*/
private RocksDBStateBackend(RocksDBStateBackend original, Configuration
config, ClassLoader classLoader) {
+ this.configuration = config;
Review comment:
Hi, @klion26 I check the code again, only the `priorityQueueStateType` is
configured from configuration, and the type from configuration will be used
first see
```
final String priorityQueueTypeString =
config.getString(TIMER_SERVICE_FACTORY);
this.priorityQueueStateType = priorityQueueTypeString.length() > 0 ?
PriorityQueueStateType.valueOf(priorityQueueTypeString.toUpperCase()) :
original.priorityQueueStateType;
```
And other parameters are all passed by the constructor, so it can work for
the issue i think. but it looks strange indeed(some follow the original
backend, some follow the config first), I will think how to make it better.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flink] Aitozi commented on a change in pull request #8479: [FLINK-11193][State Backends]Use user passed configuration overriding default configuration loading from file
Aitozi commented on a change in pull request #8479: [FLINK-11193][State
Backends]Use user passed configuration overriding default configuration loading
from file
URL: https://github.com/apache/flink/pull/8479#discussion_r286315022
##
File path:
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackend.java
##
@@ -295,6 +297,7 @@ public RocksDBStateBackend(AbstractStateBackend
checkpointStreamBackend, boolean
* @param classLoader The class loader.
*/
private RocksDBStateBackend(RocksDBStateBackend original, Configuration
config, ClassLoader classLoader) {
+ this.configuration = config;
Review comment:
Thanks @klion26 for your review. I'm not sure whether i quite get you
meaning. But IMO this construct is only for creating a re-configured copy of
the original state backend. So the configure passed in will be used first ? And
also should keep this configure to a member field to reuse it to config the
statebackend loaded from `StateBackendLoader#fromApplicationOrConfigOrDefault`.
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:
us...@infra.apache.org
With regards,
Apache Git Services
