[GitHub] [beam] kennknowles commented on a diff in pull request #23635: Reduce log flood in Python PostCommit flink task

[GitBox <gi...@apache.org>]

kennknowles commented on code in PR #23635:
URL: https://github.com/apache/beam/pull/23635#discussion_r998897966


##########
sdks/python/test-suites/portable/common.gradle:
##########
@@ -304,6 +304,13 @@ project.tasks.register("postCommitPy${pythonVersionSuffix}IT") {
         "--environment_type=LOOPBACK",
         "--temp_location=gs://temp-storage-for-end-to-end-tests/temp-it",
         "--flink_job_server_jar=${project(":runners:flink:${latestFlinkVersion}:job-server").shadowJar.archivePath}",
+        '--sdk_harness_log_level_overrides=' +
+            // suppress info level flink.runtime log flood
+            '{\\"org.apache.flink.runtime\\":\\"WARN\\",' +

Review Comment:
   This basically looks funny to me because the SDK harness is only actually used in portable mode, where there is no `org.apache.flink` namespace.
   
   So the thing we are changing is when it is run in non-portable mode, and there is not actually any SDK harness.
   
   I don't know a good solution. It is just a naming thing. Probably good to have a single flag that works now and also later. Is there not already a `--log_level_overrides` ? I guess then it is ambiguous whether you are applying it to the runner or to the SDK harness.
   
   I have no solution. This is basically OK with me, but something about it is not perfectly clean.
   
   Maybe @lukecwik has an opinion about which flags should control which log levels. I know that today we have
   
   1. Flag for the Dataflow worker
   2. Flag for the SDK harness
   
   So unfortunately neither are a good choice for sharing with other runners.
   
   I don't know if any other runner has any flag at all. My issue is that "SDK harness" is a pretty weird name for a concept that most users really don't or shouldn't care about or know exists most of the time.



##########
runners/flink/src/main/java/org/apache/beam/runners/flink/FlinkPipelineRunner.java:
##########
@@ -67,14 +68,18 @@ public class FlinkPipelineRunner implements PortablePipelineRunner {
   public FlinkPipelineRunner(
       FlinkPipelineOptions pipelineOptions, @Nullable String confDir, List<String> filesToStage) {
     this.pipelineOptions = pipelineOptions;
-    this.confDir = confDir;
+    // confDir takes precedence than pipelineOptions.getFlinkConfDir

Review Comment:
   Put this in the javadoc. A user needs to know.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org