Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
jscheffl merged PR #50425: URL: https://github.com/apache/airflow/pull/50425 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
jscheffl commented on code in PR #50425:
URL: https://github.com/apache/airflow/pull/50425#discussion_r2083506985
##
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py:
##
@@ -787,6 +803,13 @@ def remote_worker_request_shutdown(args) -> None:
help="State of the edge worker",
)
+ARG_DAEMON = Arg(
+("-D", "--daemon"), help="Daemonize instead of running in the foreground",
action="store_true"
+)
+ARG_STDERR = Arg(("--stderr",), help="Redirect stderr to this file")
+ARG_STDOUT = Arg(("--stdout",), help="Redirect stdout to this file")
+ARG_LOG_FILE = Arg(("-l", "--log-file"), help="Location of the log file")
Review Comment:
As the parameters are only used when in daemon mode, can you mention this in
the help?
```suggestion
ARG_STDERR = Arg(("--stderr",), help="Redirect stderr to this file if run in
daemon mode")
ARG_STDOUT = Arg(("--stdout",), help="Redirect stdout to this file if run in
daemon mode")
ARG_LOG_FILE = Arg(("-l", "--log-file"), help="Location of the log file if
run in daemon mode")
```
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
jscheffl commented on code in PR #50425:
URL: https://github.com/apache/airflow/pull/50425#discussion_r2083506636
##
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py:
##
@@ -517,12 +517,18 @@ def _launch_worker(args):
@providers_configuration_loaded
def worker(args):
"""Start Airflow Edge Worker."""
+if args.umask:
+umask = args.umask
+else:
+umask = conf.get("edge", "worker_umask",
fallback=settings.DAEMON_UMASK)
Review Comment:
As you create a new setting, can you also please add this to
`providers/edge3/provider.yaml` including some descriptions?
By the way in Python this if/else can be made shorter as:
```suggestion
umask = args.umask or conf.get("edge", "worker_umask",
fallback=settings.DAEMON_UMASK)
```
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
dheerajturaga commented on PR #50425: URL: https://github.com/apache/airflow/pull/50425#issuecomment-2869669362 @jscheffl , I updated the docs highlighting this feature. I also added worker umask to the args list (aligning with celery) -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
dheerajturaga commented on code in PR #50425:
URL: https://github.com/apache/airflow/pull/50425#discussion_r2083465992
##
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py:
##
@@ -787,6 +803,13 @@ def remote_worker_request_shutdown(args) -> None:
help="State of the edge worker",
)
+ARG_DAEMON = Arg(
+("-D", "--daemon"), help="Daemonize instead of running in the foreground",
action="store_true"
+)
+ARG_STDERR = Arg(("--stderr",), help="Redirect stderr to this file")
+ARG_STDOUT = Arg(("--stdout",), help="Redirect stdout to this file")
+ARG_LOG_FILE = Arg(("-l", "--log-file"), help="Location of the log file")
Review Comment:
the `run_command_with_daemon_option` works as such. When -D is unspecified
the -l/stderr/out are ignored. When running as a daemonized process, If the log
files are unspecified, it will create `airflow-edge-worker.pid,log,out,err`
files in $AIRFLOW_HOME. The difference between -l and --stdout is that -l only
captures output from the logger object, however --stdout will capture entire
stdout ( including prints)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
dheerajturaga commented on code in PR #50425:
URL: https://github.com/apache/airflow/pull/50425#discussion_r2083465992
##
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py:
##
@@ -787,6 +803,13 @@ def remote_worker_request_shutdown(args) -> None:
help="State of the edge worker",
)
+ARG_DAEMON = Arg(
+("-D", "--daemon"), help="Daemonize instead of running in the foreground",
action="store_true"
+)
+ARG_STDERR = Arg(("--stderr",), help="Redirect stderr to this file")
+ARG_STDOUT = Arg(("--stdout",), help="Redirect stdout to this file")
+ARG_LOG_FILE = Arg(("-l", "--log-file"), help="Location of the log file")
Review Comment:
the `run_command_with_daemon_option` works as such. When -D is unspecified
the -l/stderr/out are ignored. When running as a daemonized process. If the log
files are unspecified, it will create `airflow-edge-worker.pid,log,out,err`
files in $AIRFLOW_HOME. The difference between -l and --stdout is that -l only
captures output from the logger object, however --stdout will capture entire
stdout ( including prints)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
jscheffl commented on code in PR #50425:
URL: https://github.com/apache/airflow/pull/50425#discussion_r2083139539
##
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py:
##
@@ -787,6 +803,13 @@ def remote_worker_request_shutdown(args) -> None:
help="State of the edge worker",
)
+ARG_DAEMON = Arg(
+("-D", "--daemon"), help="Daemonize instead of running in the foreground",
action="store_true"
+)
+ARG_STDERR = Arg(("--stderr",), help="Redirect stderr to this file")
+ARG_STDOUT = Arg(("--stdout",), help="Redirect stdout to this file")
+ARG_LOG_FILE = Arg(("-l", "--log-file"), help="Location of the log file")
Review Comment:
What is the difference of `-l` to `--stdout/err`? Is this just a convenience
that you don't need to set both?
If these are not given, will stdout/err just be directed to /dev/null?
Should these options carry a default to make logs into the working directory
where started?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
dheerajturaga commented on code in PR #50425:
URL: https://github.com/apache/airflow/pull/50425#discussion_r2083054107
##
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py:
##
@@ -368,7 +371,8 @@ def start(self):
except EdgeWorkerVersionException:
logger.info("Version mismatch of Edge worker and Core.
Quitting worker anyway.")
finally:
-remove_existing_pidfile(self.pid_file_path)
+if not self.daemon:
Review Comment:
`run_command_with_daemon_options` likes to remove the PID once complete.
Removing this ourself will cause it to throw and exception.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Support For Edge Worker in Daemon Mode [airflow]
dheerajturaga commented on code in PR #50425:
URL: https://github.com/apache/airflow/pull/50425#discussion_r2083053821
##
providers/edge3/src/airflow/providers/edge3/cli/edge_command.py:
##
@@ -342,7 +344,8 @@ def start(self):
if e.response.status_code == HTTPStatus.NOT_FOUND:
raise SystemExit("Error: API endpoint is not ready, please set
[edge] api_enabled=True.")
raise SystemExit(str(e))
-_write_pid_to_pidfile(self.pid_file_path)
+if not self.daemon:
Review Comment:
`run_command_with_daemon_option` likes to create the pid and manage it when
running a daemonized process. We will let it control the pid creation and
removal.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
