Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
reswqa closed pull request #24127: [FLINK-34090][core] Introduce SerializerConfig URL: https://github.com/apache/flink/pull/24127 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
reswqa commented on PR #24127: URL: https://github.com/apache/flink/pull/24127#issuecomment-1905150931 closed in 7336788a9c3495745cb4b3eda7bc00d396a14b18. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
JunRuiLee commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457372715
##
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java:
##
@@ -135,8 +135,7 @@ private static ChillSerializerRegistrar
loadFlinkChillPackageRegistrar() {
//
-private final LinkedHashMap,
ExecutionConfig.SerializableSerializer>
-defaultSerializers;
+private final LinkedHashMap, SerializableSerializer>
defaultSerializers;
Review Comment:
The other changes in this class are similar.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
JunRuiLee commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457370017
##
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/kryo/KryoSerializer.java:
##
@@ -135,8 +135,7 @@ private static ChillSerializerRegistrar
loadFlinkChillPackageRegistrar() {
//
-private final LinkedHashMap,
ExecutionConfig.SerializableSerializer>
-defaultSerializers;
+private final LinkedHashMap, SerializableSerializer>
defaultSerializers;
Review Comment:
I would prefer not to make this change as it seems inconsequential and
unrelated to the scope of this PR.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
X-czh commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457342978
##
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java:
##
@@ -1097,21 +1008,6 @@ public ArchivedExecutionConfig archive() {
// -- Utilities
--
-public static class SerializableSerializer &
Serializable>
-implements Serializable {
Review Comment:
OK, I'll revert the related changes, thanks for reminding
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
X-czh commented on code in PR #24127: URL: https://github.com/apache/flink/pull/24127#discussion_r1457340110 ## flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java: ## @@ -146,6 +144,12 @@ public class ExecutionConfig implements Serializable, Archiveable
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
JunRuiLee commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457309568
##
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java:
##
@@ -146,6 +144,12 @@ public class ExecutionConfig implements Serializable,
Archiveable &
Serializable>
-implements Serializable {
Review Comment:
Given that the SerializableSerializer is part of the public API, we cannot
simply remove it. Additionally, we should avoid directly updating any return
types of Public API that use SerializableSerializer. I suggest to maintain the
current implementation without changes.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
reswqa commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457232519
##
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java:
##
@@ -74,8 +68,19 @@ public final class SerializerConfig implements Serializable {
private LinkedHashMap, Class>>
registeredTypeFactories =
new LinkedHashMap<>();
+private boolean hasGenericTypesEnabled;
+private boolean forceKryo;
+private boolean forceAvro;
+
//
+public SerializerConfig() {
Review Comment:
Through the offline discussion, I think what you did at the beginning is
more reasonable. We can revert this new commit then.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
X-czh commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457133137
##
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java:
##
@@ -74,8 +68,19 @@ public final class SerializerConfig implements Serializable {
private LinkedHashMap, Class>>
registeredTypeFactories =
new LinkedHashMap<>();
+private boolean hasGenericTypesEnabled;
+private boolean forceKryo;
+private boolean forceAvro;
+
//
+public SerializerConfig() {
Review Comment:
I just found
[FLINK-33980](https://issues.apache.org/jira/browse/FLINK-33980) was merged
yesterday. That makes things a lot easier. We can even get rid of the
`configure` method. I'll update the PR
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
X-czh commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457126011
##
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java:
##
@@ -74,8 +68,19 @@ public final class SerializerConfig implements Serializable {
private LinkedHashMap, Class>>
registeredTypeFactories =
new LinkedHashMap<>();
+private boolean hasGenericTypesEnabled;
+private boolean forceKryo;
+private boolean forceAvro;
+
//
+public SerializerConfig() {
Review Comment:
In production,` ExecutionConfig#configure` is always called AFAIK, but I'll
need to double check it. In UTs, usually it is not called, but that can be
easily fixed.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
reswqa commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457109072
##
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java:
##
@@ -74,8 +68,19 @@ public final class SerializerConfig implements Serializable {
private LinkedHashMap, Class>>
registeredTypeFactories =
new LinkedHashMap<>();
+private boolean hasGenericTypesEnabled;
+private boolean forceKryo;
+private boolean forceAvro;
+
//
+public SerializerConfig() {
Review Comment:
Whether we will always call `ExecutionConfig#configure`? If so, the default
values should not matter. If not, can we pass these arguments from the
outside? I kind of dislike creating a configuration object in this class. :)
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
X-czh commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1457027155
##
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java:
##
@@ -0,0 +1,413 @@
+/*
+ * 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.api.common.serialization;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.api.common.typeinfo.TypeInfoFactory;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.ConfigurationUtils;
+import org.apache.flink.configuration.PipelineOptions;
+import org.apache.flink.configuration.ReadableConfig;
+
+import com.esotericsoftware.kryo.Serializer;
+
+import java.io.Serializable;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * A config to define the behavior for serializers in Flink job, it manages
the registered types and
+ * serializers. The config is created from job configuration and used by Flink
to create serializers
+ * for data types.
+ */
+@PublicEvolving
+public final class SerializerConfig implements Serializable {
+
+private static final long serialVersionUID = 1L;
+
+/**
+ * In the long run, this field should be somehow merged with the {@link
Configuration} from
+ * StreamExecutionEnvironment.
+ */
+private final Configuration configuration = new Configuration();
Review Comment:
Updated
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
X-czh commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1456946751
##
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java:
##
@@ -0,0 +1,413 @@
+/*
+ * 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.api.common.serialization;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.api.common.typeinfo.TypeInfoFactory;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.ConfigurationUtils;
+import org.apache.flink.configuration.PipelineOptions;
+import org.apache.flink.configuration.ReadableConfig;
+
+import com.esotericsoftware.kryo.Serializer;
+
+import java.io.Serializable;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * A config to define the behavior for serializers in Flink job, it manages
the registered types and
+ * serializers. The config is created from job configuration and used by Flink
to create serializers
+ * for data types.
+ */
+@PublicEvolving
+public final class SerializerConfig implements Serializable {
+
+private static final long serialVersionUID = 1L;
+
+/**
+ * In the long run, this field should be somehow merged with the {@link
Configuration} from
+ * StreamExecutionEnvironment.
+ */
+private final Configuration configuration = new Configuration();
Review Comment:
Yes, it's possible to use fields. We just need to introduce a default
construct that populates the default values of the fields with the default
Configuration object.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
reswqa commented on code in PR #24127:
URL: https://github.com/apache/flink/pull/24127#discussion_r1456941270
##
flink-core/src/main/java/org/apache/flink/api/common/serialization/SerializerConfig.java:
##
@@ -0,0 +1,413 @@
+/*
+ * 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.api.common.serialization;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.api.common.typeinfo.TypeInfoFactory;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.configuration.ConfigurationUtils;
+import org.apache.flink.configuration.PipelineOptions;
+import org.apache.flink.configuration.ReadableConfig;
+
+import com.esotericsoftware.kryo.Serializer;
+
+import java.io.Serializable;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.stream.Collectors;
+
+/**
+ * A config to define the behavior for serializers in Flink job, it manages
the registered types and
+ * serializers. The config is created from job configuration and used by Flink
to create serializers
+ * for data types.
+ */
+@PublicEvolving
+public final class SerializerConfig implements Serializable {
+
+private static final long serialVersionUID = 1L;
+
+/**
+ * In the long run, this field should be somehow merged with the {@link
Configuration} from
+ * StreamExecutionEnvironment.
+ */
+private final Configuration configuration = new Configuration();
Review Comment:
Do we have to introduce a `Configuration` here? Can we use fields instead.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
flinkbot commented on PR #24127: URL: https://github.com/apache/flink/pull/24127#issuecomment-1897829521 ## CI report: * 5dacb13c87a7a1a8b957d1d93d35ff481a360cd5 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-34090][core] Introduce SerializerConfig [flink]
X-czh opened a new pull request, #24127: URL: https://github.com/apache/flink/pull/24127 ## What is the purpose of the change Introduce SerializerConfig for serializers decouple the serializer from ExecutionConfig. ## Brief change log Introduce SerializerConfig and wire serializer-related methods in ExecutionConfig to it. ## Verifying this change This change is already covered by existing tests: ExecutionConfigTest. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (yes / no) no - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / no) yes - The serializers: (yes / no / don't know) no - The runtime per-record code paths (performance sensitive): (yes / no / don't know) no - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know) no - The S3 file system connector: (yes / no / don't know) no ## Documentation - Does this pull request introduce a new feature? (yes / no) no - If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) not applicable -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
