patgarz commented on PR #34051: URL: https://github.com/apache/airflow/pull/34051#issuecomment-1720389299
@uranusjr After some review, testing, and thinking on it, I think it may be a bad idea to set a default value here on `owner_display_name` in isolation. The `Log` table does not currently have any properties set aside from `primary_key` on the `id` column: https://github.com/apache/airflow/blob/main/airflow/models/log.py#L32-L40. Setting a default value for one column (when others are frequently `NULL`, and even `owner` itself has the possibility of being `null` even though it shouldn't ever be) is inconsistent and raises more questions than it solves, in my opinion. Second, the `Log` table isn't responsible for any application logic; it's simply a friendlier-than-raw-text table to log user & system actions. Setting default or required values goes against that spirit as we want to log *exactly* the action that was taken with it associated data, and not munge that data as that makes troubleshooting (and auditing generally) harder. (Obviously, an empty string rather than NULL is fairly safe in `owner_display_name`, but I don't know if that holds for other columns, and it does make queries harder from a consistency standpoint.) If we wanted to consider a more substantive rewrite of the `Log` model such that it did have `default` and `nullable` specifiers on applicable columns, I think this could be a worthwhile change. But I think that deserves a separate PR (that I'm happy to put together, though I'm not sure it solves more than it possibly breaks since the Log table isn't responsible for any application logic and is really just a friendlier mechanism when compared to a raw text log). As such I think the current state of the PR is the desired state (assuming tests pass). Let me know your thoughts. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
