Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21613 )
Change subject: [log] Support logging audit logs to a separate file ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/21613/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21613/5//COMMIT_MSG@15 PS5, Line 15: In this patch, I : implemented this feature to redirect audit logs. There are different flavors of auditing, and sometimes they enforce to provide at least 'object', 'subject', and 'action' items, so it's easy to automatically parse such logs. Do you it makes sense to have something similar here, or there is no need for that? http://gerrit.cloudera.org:8080/#/c/21613/5//COMMIT_MSG@19 PS5, Line 19: to the : audit log Is it possible to keep a record about those operations in the INFO log as well? That would be beneficial from the troubleshooting perspective if it's necessary to build a sequence of events for a particular time window. It's possible to do so now having just the INFO log, and it would be great to keep this even if writing audit-related records into a separate log file. http://gerrit.cloudera.org:8080/#/c/21613/5//COMMIT_MSG@55 PS5, Line 55: kudu_master.7732168b9e26.root.AuditLog.20240808-170313.0.25681. Is it's assumed that all the audit logs of any severity (INFO, WARNING, etc.) are ending up in this single file or the are different per-severity audit logs, similar to what's available for regular INFO/WARNING/etc logs? http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.h File src/kudu/master/audit_logger.h: http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.h@82 PS5, Line 82: AuditLogSink * nit: correct this to be compliant with the coding style http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.cc File src/kudu/master/audit_logger.cc: http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.cc@38 PS5, Line 38: master_audit_log_record_independently Why is 'master' relevant to audit logging in this generic implementation? http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.cc@39 PS5, Line 39: independently nit: 'independent' seems to be a misnomer in this context, consider 'separate' http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.cc@42 PS5, Line 42: This prevents the audit logs from " : "being insufficient for issue diagnosis due to the regular logs " : "rolling over too quickly. Consider moving this part into the description of the commit. http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.cc@62 PS5, Line 62: kudu_master Why is this hard-coded? http://gerrit.cloudera.org:8080/#/c/21613/5/src/kudu/master/audit_logger.cc@63 PS5, Line 63: auto l = make_unique<RollingLog>(Env::Default(), log_dir_, program_name_, "AuditLog"); Why to allocate this member field on the heap? Why not to allocate it on the stack if this is done in the constructor? -- To view, visit http://gerrit.cloudera.org:8080/21613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie5323361befb456d91a12da7273865542f1d2430 Gerrit-Change-Number: 21613 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 08 Aug 2024 19:19:33 +0000 Gerrit-HasComments: Yes
