Support dynamic enable/disable audit log #17489
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jiacheliu3 Would you mind helping take a look? I'm not sure this would have performance issue if we start audit writer at the very beginning |
dbw9580
reviewed
Jun 9, 2023
| ? mAsyncAuditLogWriter.getAuditLogEntriesSize() : -1); | ||
| } | ||
| mAsyncAuditLogWriter = new AsyncUserAccessAuditLogWriter("AUDIT_LOG"); | ||
| mAsyncAuditLogWriter.start(); |
There was a problem hiding this comment.
This can be initialized in the constructor
jiacheliu3
requested changes
Jun 16, 2023
There was a problem hiding this comment.
I think I can understand what this is trying to do and it makes sense to me. However, I figure we need:
- Some comments in the changed code sections to explain why the audit logger can be always running and how
DefaultFileSystemMaster.createAuditContext()checksMASTER_AUDIT_LOGGING_ENABLEDand does/doesn't write audit log. - Some UTs to verify our understanding is correct
Could you add those? Thanks!
jiacheliu3
requested changes
Jul 14, 2023
Comment on lines
538
to
542
| /** | ||
| * As we can determine whether enable the audit log through update the config, | ||
| * and check it in {@link #createAuditContext}, | ||
| * so we need this audit log writer thread all the time. | ||
| */ |
There was a problem hiding this comment.
Suggested change
| /** | |
| * As we can determine whether enable the audit log through update the config, | |
| * and check it in {@link #createAuditContext}, | |
| * so we need this audit log writer thread all the time. | |
| */ | |
| /** | |
| * The audit logger will be running all the time, and an operation checks whether or not | |
| * to enable audit logs in {@link #createAuditContext}. So audit log can be turned on/off | |
| * at runtime by updating the property key. | |
| */ |
There was a problem hiding this comment.
similarly pls update the other comments
Comment on lines
809
to
812
| if (mAsyncAuditLogWriter != null) { | ||
| mAsyncAuditLogWriter.stop(); | ||
| mAsyncAuditLogWriter = null; | ||
| } |
There was a problem hiding this comment.
Stop the logger in stop(). You may start it again in start()?
There was a problem hiding this comment.
It make me confusing, it remove it and never stop it for now.
dbw9580
suggested changes
Jul 14, 2023
Comment on lines
+571
to
+572
| () -> mAsyncAuditLogWriter != null | ||
| ? mAsyncAuditLogWriter.getAuditLogEntriesSize() : -1); |
There was a problem hiding this comment.
the null check is not necessary
|
@jiacheliu3 I've updated the comments, Would you please take another look? |
|
@jiacheliu3 Thanks for your review, would you like to take another look? |
jiacheliu3
added
the
type-ease-of-use
|
alluxio-bot, merge this please |
alluxio-bot
added a commit
that referenced
this pull request
Nov 8, 2023
### What changes are proposed in this pull request? Merge missing commits from master-2.x to main. The commits in 2023/07/01~2023/11/08 from main...master-2.x will be included by this PR. We do this merge to catch missing fixes from `master-2.x` and catch the train before `main` cuts a release. #17747 is not cherry picked because tencent cloud EMR doc is removed #17755 is not cherry picked because DistLoadCliRunner has been removed in 3.x #17758 is not cherry picked because MonoBlockStore has been removed in 3.x #17641 is not cherry picked because the PR has already been in main #17781 is not cherry picked because the PR has already been in main #17722 is not cherry picked because the alluxio-fuse command has been changed a lot #17489 is not cherry picked because audit log on master is no longer in 3.x #17865 is not cherry picked because replication on job service is no longer in 3.x #17858 is not cherry picked because it is already in main #18090 is not cherry picked because generate-tarball has been rewritten in 3.x #18091 is not cherry picked because the change is already in main #17474 is not cherry picked because reconfiguration feature is not defined in 3.x #17735 is not cherry picked because MonoBlockStore is no longer in 3.x #18133 is not cherry picked because the issue is about master metadata and no longer relevant in 3.x #17910 is not cherry picked because I prefer to do that manually #17983 is not cherry picked because the web UI has been reworked #17984 is not cherry picked because Mount/Unmount commands have been reworked in 3.x #18103 is not cherry picked because worker cache metrics have been reworked in 3.x #18185 is not cherry picked because the report command has been reworked in 3.x #18222 is not cherry picked because Mount/Unmount operations have been reworked in 3.x #18143 is not cherry picked because the change is already in main #18303 is not cherry picked because the change is already in main #18208 is not cherry picked because cache metrics have been reworked in 3.x #17002 is not cherry picked because the owner has been notified separately #18334 is not cherry picked because the bash scripts have been reworked in 3.x #18326 is not cherry picked because the owner has been notified separately pr-link: #18397 change-id: cid-dbf8cbb2d9e721a5a0a1e5028a3c9577438a2ac0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Support dynamic enable/disable audit log
Why are the changes needed?
Now, if we disable auditlog before master startup, we cannot support enable it dynamically, but it can be disable if it is enabled at startup time, it is tricky.
And it is not expensive to start an object.
Does this PR introduce any user facing changes?
No