[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #202: [FLINK-27483]Add session job config field

2022-05-13 Thread GitBox 


Aitozi commented on code in PR #202:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/202#discussion_r872916175


##
docs/content/docs/operations/configuration.md:
##
@@ -53,3 +53,12 @@ To learn more about metrics and logging configuration please 
refer to the dedica
 ## Operator Configuration Reference
 
 {{< generated/kubernetes_operator_config_configuration >}}
+
+## Job Specific Configuration Reference
+Job specific configuration can be configured under `spec.flinkConfiguration` 
and it will override flink configurations defined in `flink-conf.yaml`.
+
+- For application clusters, `spec.flinkConfiguration` will be located in 
`FlinkDeployment` CustomResource.
+- For session clusters, configuring `spec.flinkConfiguration` in parent 
`FlinkDeployment` will be applied to all session jobs within the session 
cluster.
+  - You can configure some additional job specific supplemental configuration 
through `spec.flinkConfiguration` in `FlinkSessionJob` CustomResource. 
+  Those session job level configurations will override the parent session 
cluster's Flink configuration. Please note only the following configurations 
are considered to be valid configurations.
+- `kubernetes.operator.user.artifacts.http.header`

Review Comment:
   I think it's much clear now . 
   Some other thoughts come to my mind that, Maybe we could extend the 
`ConfigOption` to mark the config belong to which component (flink/operator). 
It's out of this PR, we could do it later if necessary.



-- 
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

[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #202: [FLINK-27483]Add session job config field

2022-05-13 Thread GitBox 


Aitozi commented on code in PR #202:
URL: 
https://github.com/apache/flink-kubernetes-operator/pull/202#discussion_r872450640


##
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/artifact/HttpArtifactFetcher.java:
##
@@ -32,12 +37,24 @@ public class HttpArtifactFetcher implements ArtifactFetcher 
{
 public static final HttpArtifactFetcher INSTANCE = new 
HttpArtifactFetcher();
 
 @Override
-public File fetch(String uri, File targetDir) throws Exception {
+public File fetch(String uri, Configuration flinkConfiguration, File 
targetDir)
+throws Exception {
 var start = System.currentTimeMillis();
 URL url = new URL(uri);
+HttpURLConnection conn = (HttpURLConnection) url.openConnection();
+
+Map clusterLevelHeader =

Review Comment:
   we do not know this header is cluster level or job level here, what about 
naming it `headers`  directly ?



##
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/AbstractFlinkSpec.java:
##
@@ -40,4 +42,7 @@ public abstract class AbstractFlinkSpec {
  * restart, change the number to anything other than the current value.
  */
 private Long restartNonce;
+
+/** Flink configuration overrides for the Flink deployment. */

Review Comment:
   nit: the comment should be update: overrides for the Flink deployment or 
session job.



##
docs/content/docs/operations/configuration.md:
##
@@ -53,3 +53,11 @@ To learn more about metrics and logging configuration please 
refer to the dedica
 ## Operator Configuration Reference
 
 {{< generated/kubernetes_operator_config_configuration >}}
+
+## Job Specific Configuration Reference
+Job specific configuration can be configured under 
`spec.job.flinkConfiguration` and it will override flink configurations defined 
in `flink-conf.yaml`.
+
+- For application clusters, `spec.job.flinkConfiguration` will be located in 
`FlinkDeployment` CustomResource.
+- For session clusters, configuring `spec.job.flinkConfiguration` in parent 
`FlinkDeployment` will be applied to all session jobs within the session 
cluster.
+You can also configure `spec.job.flinkConfiguration` in `FlinkSessionJob` 
CustomResource for a specific session job. 
+The session job level configuration will override the parent session cluster's 
Flink configuration.

Review Comment:
   I think we should let user know that the cluster level's flink config will 
not overridden by the session job's flinkConfiguration actually (I mean the 
session cluster's config). Personally, I think what 
sessionjob`spec.flinkConfiguration` defined, is not the **flink** 
Configuration, just some custom configs. 



##
docs/content/docs/operations/configuration.md:
##
@@ -53,3 +53,11 @@ To learn more about metrics and logging configuration please 
refer to the dedica
 ## Operator Configuration Reference
 
 {{< generated/kubernetes_operator_config_configuration >}}
+
+## Job Specific Configuration Reference
+Job specific configuration can be configured under 
`spec.job.flinkConfiguration` and it will override flink configurations defined 
in `flink-conf.yaml`.
+
+- For application clusters, `spec.job.flinkConfiguration` will be located in 
`FlinkDeployment` CustomResource.

Review Comment:
   ditto



##
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/sessionjob/FlinkSessionJobReconciler.java:
##
@@ -83,6 +84,13 @@ public void reconcile(FlinkSessionJob flinkSessionJob, 
Context context) throws E
 
 Configuration deployedConfig = 
configManager.getObserveConfig(flinkDepOptional.get());
 
+// merge session job specific config
+Map sessionJobFlinkConfiguration =

Review Comment:
   nit: maybe we could move this to the `FlinkConfigManager` 



##
docs/content/docs/operations/configuration.md:
##
@@ -53,3 +53,11 @@ To learn more about metrics and logging configuration please 
refer to the dedica
 ## Operator Configuration Reference
 
 {{< generated/kubernetes_operator_config_configuration >}}
+
+## Job Specific Configuration Reference
+Job specific configuration can be configured under 
`spec.job.flinkConfiguration` and it will override flink configurations defined 
in `flink-conf.yaml`.
+
+- For application clusters, `spec.job.flinkConfiguration` will be located in 
`FlinkDeployment` CustomResource.
+- For session clusters, configuring `spec.job.flinkConfiguration` in parent 
`FlinkDeployment` will be applied to all session jobs within the session 
cluster.
+You can also configure `spec.job.flinkConfiguration` in `FlinkSessionJob` 
CustomResource for a specific session job. 

Review Comment:
   A newline here 



##
docs/content/docs/operations/configuration.md:
##
@@ -53,3 +53,11 @@ To learn more about metrics and logging configuration please 
refer to the dedica
 ## Operator Configuration