Enable accountsdb_scan_account_storage_no_bank tests for hot storage by yhchiang-sol · Pull Request #344 · anza-xyz/agave

yhchiang-sol · 2024-03-20T19:13:49Z

Problem

test_accountsdb_scan_account_storage_no_bank_one_slot and
test_accountsdb_scan_account_storage_no_bank only run for AppendVec.

Summary of Changes

Enable the above two tests to cover both AppendVec and HotStorage.

Test Plan

Run the newly enabled tests.

Dependency

#334, #446

yhchiang-sol · 2024-03-20T19:16:08Z

+            // promote this to panic soon.
+            // disabled due to many false positive warnings while running tests.
+            // blocked by rpc's upgrade to jsonrpc v17
+            /*
            panic!(
                "TieredStorage failed to remove backing storage file '{}': {err}",
                self.path.display(),
-            );
+            );*/
+            inc_new_counter_info!("append_vec_drop_fail", 1);


This section was copied from the AppendVec. The test will fail without this as some tests sometimes create an empty AccountsFile instance without writing it causing nothing to be dropped.

Which test(s) fail? Can we usually ManuallyDrop, like we do in the append vec tests here?

agave/accounts-db/src/append_vec.rs

Lines 1205 to 1206 in b2f4fb3

// wrap AppendVec in ManuallyDrop to ensure we do not remove the backing file when dropped

let av = ManuallyDrop::new(AppendVec::new(path, true, 1024 * 1024));

Also, when we talked about this offline, can you check if the error is for "file not found" or not? If the error is something else, maybe panic still is the right call.

The counter name should not be "append_vec" though.

The enabled tests here will fail because of this. But I think skipping no-such-file-or-directory error should address the problem as we previously discussed.

Manual-drop works as well.

thread 'accounts_db::tests::test_accountsdb_scan_account_storage_no_bank_one_slot' panicked at accounts-db/src/tiered_storage.rs:67:13: TieredStorage failed to remove backing storage file '/var/folders/2_/848_htk13yb0b_r0wg105t400000gn/T/.tmpFVnzMY/run/0.0': No such file or directory (os error 2)

Fixed in #446 that keeps the panic behavior but exclude io::ErrorKind::NotFound.

codecov-commenter · 2024-03-20T21:07:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.8%. Comparing base (79e316e) to head (9b60781).
⚠️ Report is 6992 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #344   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         843      843           
  Lines      228586   228677   +91     
=======================================
+ Hits       187124   187209   +85     
- Misses      41462    41468    +6

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

yhchiang-sol · 2024-03-21T19:13:58Z

+    #[test]
+    fn test_accountsdb_scan_account_storage_no_bank() {
+        accountsdb_scan_account_storage_no_bank_test_case::<WritableAppendVecProvider>();
+        accountsdb_scan_account_storage_no_bank_test_case::<WritableHotStorageProvider>();
+    }
+


@brooksprumo: Just want to get quick feedback from you. Here's the way that I am currently developing to enable accounts-db tests for both AppendVec and TieredStorage. Does it look good to you?

You may be able to use test_case to generate these easier. It'll also then create two proper tests, so we can see pass/fail on each accounts file type separately.

Here's an example with generics: https://github.com/frondeus/test-case/blob/master/tests/acceptance_cases/cases_support_generics/src/lib.rs

Neat! This is great! Thanks for the pointer!

Changed the test to use test_case

brooksprumo

I can't think of anything better, so seems good for now. The non-test code should be monomorphized and the compiler should optimize any indirection away (I believe/hope!).

brooksprumo · 2024-03-21T19:30:11Z

+            // promote this to panic soon.
+            // disabled due to many false positive warnings while running tests.
+            // blocked by rpc's upgrade to jsonrpc v17
+            /*
            panic!(
                "TieredStorage failed to remove backing storage file '{}': {err}",
                self.path.display(),
-            );
+            );*/
+            inc_new_counter_info!("append_vec_drop_fail", 1);


Also, when we talked about this offline, can you check if the error is for "file not found" or not? If the error is something else, maybe panic still is the right call.

brooksprumo · 2024-03-21T19:30:26Z

    footer::{AccountBlockFormat, AccountMetaFormat},
    hot::{HotStorageWriter, HOT_FORMAT},
    index::IndexBlockFormat,
+    log::log_enabled,


What's this for?

Was for the log. Removed

brooksprumo · 2024-03-21T19:30:46Z

+            // promote this to panic soon.
+            // disabled due to many false positive warnings while running tests.
+            // blocked by rpc's upgrade to jsonrpc v17
+            /*
            panic!(
                "TieredStorage failed to remove backing storage file '{}': {err}",
                self.path.display(),
-            );
+            );*/
+            inc_new_counter_info!("append_vec_drop_fail", 1);


The counter name should not be "append_vec" though.

brooksprumo · 2024-03-21T19:50:09Z

+    #[test]
+    fn test_accountsdb_scan_account_storage_no_bank() {
+        accountsdb_scan_account_storage_no_bank_test_case::<WritableAppendVecProvider>();
+        accountsdb_scan_account_storage_no_bank_test_case::<WritableHotStorageProvider>();
+    }
+


You may be able to use test_case to generate these easier. It'll also then create two proper tests, so we can see pass/fail on each accounts file type separately.

Here's an example with generics: https://github.com/frondeus/test-case/blob/master/tests/acceptance_cases/cases_support_generics/src/lib.rs

brooksprumo · 2024-03-21T19:54:12Z


 impl AccountStorageEntry {
-    pub fn new(path: &Path, slot: Slot, id: AppendVecId, file_size: u64) -> Self {
+    pub fn new<T: WritableAccountsFileProvider>(


Please only use T when it's a container and the container can hold any arbitrary type.

How about something like AF instead?

Done. Used AP (stands for AccountsFile Provider).

yhchiang-sol · 2024-03-26T18:23:15Z

Rebased on top of the updated #334. Used test_case. Renamed T -> AP. Bypass file-not-found error on TieredStorage::drop().

yhchiang-sol · 2024-03-27T05:58:59Z

Rebased. Now it depends on #334 and #446.

jeffwashington · 2024-04-01T21:58:22Z

@brooksprumo I think the other changes are no longer part of this pr.

brooksprumo

To make sure this PR still built, I checked it out and rebased on master. It successfully passed cargo clippy --tests.

HaoranYi

lgtm

yhchiang-sol marked this pull request as draft March 20, 2024 19:13

yhchiang-sol commented Mar 20, 2024

View reviewed changes

yhchiang-sol commented Mar 21, 2024

View reviewed changes

brooksprumo reviewed Mar 21, 2024

View reviewed changes

brooksprumo mentioned this pull request Mar 26, 2024

Introduce AccountsFileProvider #334

Closed

yhchiang-sol force-pushed the ts-scan_account_storage-test branch from 02462f9 to ec97e39 Compare March 26, 2024 18:21

yhchiang-sol force-pushed the ts-scan_account_storage-test branch 3 times, most recently from 351322c to bdba36e Compare March 27, 2024 05:56

mergify Bot added community need:merge-assist labels Mar 31, 2024

Enable accountsdb_scan_account_storage_no_bank tests for hot storage.

9b60781

jeffwashington force-pushed the ts-scan_account_storage-test branch from bdba36e to 9b60781 Compare April 1, 2024 21:49

jeffwashington marked this pull request as ready for review April 1, 2024 21:58

jeffwashington requested a review from brooksprumo April 1, 2024 21:58

jeffwashington requested a review from HaoranYi April 1, 2024 22:17

brooksprumo approved these changes Apr 2, 2024

View reviewed changes

jeffwashington merged commit e815925 into anza-xyz:master Apr 2, 2024

HaoranYi reviewed Apr 2, 2024

View reviewed changes

brooksprumo mentioned this pull request Apr 2, 2024

Sets write version to 0 when storing accounts #476

Merged

	// wrap AppendVec in ManuallyDrop to ensure we do not remove the backing file when dropped
	let av = ManuallyDrop::new(AppendVec::new(path, true, 1024 * 1024));

Conversation

yhchiang-sol commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Test Plan

Dependency

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhchiang-sol Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhchiang-sol commented Mar 26, 2024

Uh oh!

yhchiang-sol commented Mar 27, 2024

Uh oh!

jeffwashington commented Apr 1, 2024

Uh oh!

brooksprumo left a comment

Choose a reason for hiding this comment

Uh oh!

HaoranYi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yhchiang-sol commented Mar 20, 2024 •

edited

Loading

yhchiang-sol Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 20, 2024 •

edited

Loading