could you move it before L8? I think the file should be sorted based on the feature added.

I am not sure, but I thought the list is sorted by descending chronological order?

Oh sry I didn't know that, if that is the case I'll update the order.

n00b qq: are these mode options defined in RotatingFileHandler?

sry I didn't get ur question... Are you asking what are these options for? That is the file opening mode, set to 'a' so we keep appending to the file( so we don't overwrite the log if we restart the scheduler or so). 'a' is indeed the default mode but just wanted to keep it here to make it more clear

nit: constant for 777 and qq: why set 777 permission?

My bad here, miss read the comment from here and thought I need 777 for the mkdirs to work. Will update it to 755.

Rather than checking for if the path already exists it's better to try to create and catching the FileExists error - we had a race condition else where (in the utils.file_handler or something) that was fixed by doing this.

It's less likely to apply here, but better safe than sorry/

You are perfectly correct. Did that before but forgot about it entirely :P

Actually our mkdirs util method already covered this... Will just mkdirs(directory, 0o755)

maybe a comment here, and how we do handle this kinda version dependent change at other places in airflow?

Definitely, I'll put comment in next iteration. We have a couple of places also branching based on sys.version_info( mostly because of the str to unicode mess).

saw this change #3298 and will try to use that more decent approach

same, why not namedTuple?

This is definitely a better candidate for namedTuple, will try update. Ty.

This is either some Immutable Singleton or

DagParsingSignal = namedtuple(
    'DagParsingSignal',
    'AGENT_HEARTBEAT,   MANAGER_DONE,   TERMINATE_MANAGER,   END_MANAGER')(
    'agent_heartbeat', 'manager_done', 'terminate_manager', 'end_manager')

Or Python3 Enum, with https://pypi.org/project/enum34/ as the backport

👍 @feng-tao had the same comment and will try use namedtuple. Ty!

Hi @yrqls21 , this may be an invalid question: may you advise what this environment variable CONFIG_PROCESSOR_MANAGER_LOGGER is meant to be? Is it going to be specified by the user, or it's environment-specific?

It's not documented anywhere else.

Turns out I DID ask an invalid question.... I have noticed the relevant chunk in airflow/utils/dag_processing.py.....

So this environment variable is some sort of "flag" to help determine if these a few following lines here in airflow/config_templates/airflow_local_settings.py should be run when we load/reload airflow_local_settings, if I'm not mistaken?

This feels like it should be a config option somewhere in the config, and then the normal AIRFLOW__${SECT}__${OPT} environment variables could be used.

Also there is zero documentation or metion anywher eelse what this block is doing. Adding to the default config (with comments explaining it's use) gives us two birds with one stone.

@XD-DENG you are perfectly correctly. The purpose is to avoid RotatingFileHandler to be initialized in multiple processes, which would cause exceptions. I thought I had some comments around it but obviously I'm wrong :(

@ashb This is more like a internal flag controlling the behavior of different processes, similar idea as this line https://github.com/apache/incubator-airflow/blob/master/airflow/bin/cli.py#L868, I'll definitely add comments here.

Nit: "Exiting gracefully after receiving signal {}" please

nit: following

Or Python3 Enum, with https://pypi.org/project/enum34/ as the backport

According to the docs:

If timeout is None then an infinite timeout is used.

So I thine we can do self._parent_signal_conn.poll(None) and then the kernel/OS will deal with waking us when there's something to read, and we wouldn't need a sleep here either.

I think.

Alternatively, an even simpler approach: Just call recv() - that will block until there is something to read. No poll or sleep needed that way.

You are perfectly correct. Totally forget about recv() is blocking. Will update to just use recv(). Ty!

This reload is done soley to reconfigure the logger right? Wouldn't it be more immediate and less "hacky" (using reload is a bit of a kludge) to reconfigure the logger instead?

Or even better: could we use a different logger name and have it already configured with normal logging, but just only used by this class/logger?

W/o reloading airflow_local_settings, the custom logic here would not be evaluated and thus we'll end up reconfigure with the same logging config again...

Also as mentioned in the comment, right now the major purposes of reloading settings is to reload logger and also the connection pool.

Tho I do agree reload is a bit of a kludge--I would avoid it if I had other solutions :(

Can you do something like this?

logging.config.dictConfig({
  # New handler,
  disable_existing_loggers=False
}

Nice trick here 👍 But moving the def of handler and logger here would prevent people from using there custom handler and logger for DAG parsing right?(e.g. logger with both processor_manager and console handler) Also we've recently encountered a rare case that same logger on multiple process would create lock issue if we do not reload and thus it feel safer to just reload all loggers.

Minor nit: Since python2 is going away in less than a year can we reverse this:

i.e. In py2 re-define range = xrange and then just use range here.

👍

Nit: "Sending termination message" - signal could be confused with OS level signals, but this is a message/request.

Does this timeout need to be configureable?

I'll be adding a TODO comment here to make it aligned with the existing code. I might later on start another discussion around this magic number.

Nit: "upon receiving signal" (the same in both cases of this please)

Is this another magic number?

Yes it is. Will put a similar TODO here and revisit it together with termination timeout magic number later.