Enable accountsdb_scan_account_storage_no_bank tests for hot storage #344

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

accounts-db/src/tiered_storage.rs

+            // 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 7 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

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

accounts-db/src/accounts_db.rs

+    #[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

accounts-db/src/accounts_db.rs

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

accounts-db/src/tiered_storage.rs

+            // 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

accounts-db/src/tiered_storage.rs

@@ -22,6 +22,7 @@ use {
    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

accounts-db/src/tiered_storage.rs

+            // 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

accounts-db/src/accounts_db.rs

+    #[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

accounts-db/src/accounts_db.rs

@@ -1031,10 +1030,15 @@ pub struct AccountStorageEntry {
 }

 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
37 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable accountsdb_scan_account_storage_no_bank tests for hot storage #344

Enable accountsdb_scan_account_storage_no_bank tests for hot storage #344

yhchiang-sol commented Mar 20, 2024 •

edited

Loading

yhchiang-sol Mar 20, 2024

brooksprumo Mar 21, 2024

brooksprumo Mar 21, 2024

brooksprumo Mar 21, 2024

yhchiang-sol Mar 21, 2024 •

edited

Loading

yhchiang-sol Mar 27, 2024

codecov-commenter commented Mar 20, 2024 •

edited

Loading

yhchiang-sol Mar 21, 2024

brooksprumo Mar 21, 2024

yhchiang-sol Mar 21, 2024

yhchiang-sol Mar 27, 2024

brooksprumo left a comment

brooksprumo Mar 21, 2024

brooksprumo Mar 21, 2024

yhchiang-sol Mar 26, 2024

brooksprumo Mar 21, 2024

brooksprumo Mar 21, 2024

brooksprumo Mar 21, 2024

yhchiang-sol Mar 27, 2024

yhchiang-sol commented Mar 26, 2024

yhchiang-sol commented Mar 27, 2024

jeffwashington commented Apr 1, 2024

brooksprumo left a comment

HaoranYi left a comment

	// 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));

Enable accountsdb_scan_account_storage_no_bank tests for hot storage #344

Enable accountsdb_scan_account_storage_no_bank tests for hot storage #344

Conversation

yhchiang-sol commented Mar 20, 2024 • edited Loading

Problem

Summary of Changes

Test Plan

Dependency

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhchiang-sol Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhchiang-sol commented Mar 26, 2024

yhchiang-sol commented Mar 27, 2024

jeffwashington commented Apr 1, 2024

brooksprumo left a comment

Choose a reason for hiding this comment

HaoranYi left a comment

Choose a reason for hiding this comment

yhchiang-sol commented Mar 20, 2024 •

edited

Loading

yhchiang-sol Mar 21, 2024 •

edited

Loading

codecov-commenter commented Mar 20, 2024 •

edited

Loading