Fix audit_storage issues #9265
Merged
Fix audit_storage issues #9265
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
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Doxense CI Report for Windows 10
|
saintstack
reviewed
Jan 28, 2023
| const UID id = UID::fromString(tokens[3].toString()); | ||
| AuditStorageState res = wait(getAuditState(cx, type, id)); | ||
| printf("Audit result is:\n%s", res.toString().c_str()); | ||
| } else if (tokencmp(tokens[2], "recent")) { | ||
| const int count = std::stoi(tokens[3].toString()); | ||
| int count = 5; |
There was a problem hiding this comment.
What is this? 5 'rows'? We should add this to the help output, that 5 is the default 'Count' (I can do that over in #9266).
There was a problem hiding this comment.
I think default should be ROW_LIMIT_UNLIMITED -- I think that would make more sense to the user. What do you think ?
There was a problem hiding this comment.
That's a good point, I put CLIENT_KNOBS->TOO_MANY as the default.
There was a problem hiding this comment.
Is TOO_MANY too small? Its only 1M? Thanks.
There was a problem hiding this comment.
My current plan is to keep only the most recent N audit records per audit type, I feel N would be smaller than 1M, and a background process will be added to GC the earlier audit results.
halfprice
previously approved these changes
Jan 28, 2023
|
What will the user experience be? When the audit completes, its id is cleaned. If user queries for the state of their audit, what will they get? Thanks. |
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
get_audit_status.
|
At this moment, all the audit results are kept forever, in the future, I will add a background GC process to keep only the recent |
Doxense CI Report for Windows 10
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Monterey 12.x
|
Result of foundationdb-pr-macos on macOS Monterey 12.x
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
|
Talked offline w/ @liquid-helium . +1 on this PR. Cleanup and 'count' to be addressed in follow-ons. |
saintstack
approved these changes
Jan 31, 2023
sfc-gh-nwijetunga
added a commit
to sfc-gh-nwijetunga/foundationdb
that referenced
this pull request
Feb 1, 2023
* main: (22 commits) move feed cleanup check to after data is guaranteed to be available for granule (apple#9283) remove test timeout Reduce logging level for verbose events Added documentation for consistencyscan CLI command. Fix audit_storage issues (apple#9265) Update bindings/bindingtester/spec/tenantTester.md Update bindings/bindingtester/spec/tenantTester.md update bindingtester spec Fixing SkewedReadWrite to load its metadata in a transactionally consistent way (apple#9274) push string onto stack when active tenant is set Add comments on why custom encoding is needed patch to fix some existing bindingtester issues add arg and return type to the c_api for impl.py Fix includes Add from_7.0.0_until_7.2.0 for UpgradeAndBackupRestore tests Change UpgradeAndBackupRestore to from_7.2.4 Add a new toml option to disable failure injection workload Change SubmitBackup to only reboot in Attrition add method to return idfuture add to java and python stack tester ...
sfc-gh-nwijetunga
added a commit
to sfc-gh-nwijetunga/foundationdb
that referenced
this pull request
Feb 2, 2023
* main: (23 commits) Handle EKP Tenant Not Found Errors (apple#9261) move feed cleanup check to after data is guaranteed to be available for granule (apple#9283) remove test timeout Reduce logging level for verbose events Added documentation for consistencyscan CLI command. Fix audit_storage issues (apple#9265) Update bindings/bindingtester/spec/tenantTester.md Update bindings/bindingtester/spec/tenantTester.md update bindingtester spec Fixing SkewedReadWrite to load its metadata in a transactionally consistent way (apple#9274) push string onto stack when active tenant is set Add comments on why custom encoding is needed patch to fix some existing bindingtester issues add arg and return type to the c_api for impl.py Fix includes Add from_7.0.0_until_7.2.0 for UpgradeAndBackupRestore tests Change UpgradeAndBackupRestore to from_7.2.4 Add a new toml option to disable failure injection workload Change SubmitBackup to only reboot in Attrition add method to return idfuture ...
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.
Some quick fixes, the complete fix is in #9264
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branchormainif this is the youngest branch)