Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented store module for near-network #7010

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Conversation

pompon0
Copy link
Contributor

@pompon0 pompon0 commented Jun 10, 2022

It consists of 2 layers:

  1. schema definition layer - enforcing type-safe access to the DB columns
  2. atomic access layer - defining all DB transactions, which are the high level operations that business logic can execute.

@pompon0 pompon0 requested a review from a team as a code owner June 10, 2022 07:50
@pompon0 pompon0 requested a review from mzhangmzz June 10, 2022 07:50
@pompon0 pompon0 changed the base branch from master to gprusak-time June 10, 2022 07:50
@pompon0 pompon0 requested review from mm-near and removed request for mzhangmzz June 10, 2022 07:50
chain/network/src/store/mod.rs Outdated Show resolved Hide resolved
chain/network/src/store/schema/mod.rs Outdated Show resolved Hide resolved
chain/network/src/store/schema/mod.rs Outdated Show resolved Hide resolved
chain/network/src/store/schema/mod.rs Outdated Show resolved Hide resolved
&mut self,
account_id: &AccountId,
aa: &AnnounceAccount,
) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debating whether store::Result<()> alias would make sense 🤔

If many higher-level functions can only fail with store-related errors, it would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that all the functions in a module return the same error type is a coincidence. In this particular implementation they effectively aggregate all the error variants, but I don't want it to be a rule.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that all the functions in a module return the same error type is a coincidence.

Not always: "the only way this can fail is due to error X" can be a powerful architectural invariant to keep unwanted dependencies outside the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the only way this can fail is due to error Y, which is more specific than X" is an even more powerful invariant. It is terribly annoying when a function returns an error which has variants, which can never happen (and it is usually even documented that they never happen, but authors were too lazy to make the error more specific).

Also the invariant "the only way this can fail is due to error X" becomes architecturally useless (equivalent to anyhow::Error) as soon as it is possible to embed an arbitrary error in X (std::io::Error is a great example of that).

chain/network/src/store/mod.rs Show resolved Hide resolved
chain/network/src/store/mod.rs Outdated Show resolved Hide resolved
chain/network/src/store/mod.rs Outdated Show resolved Hide resolved
chain/network-primitives/src/network_protocol/edge.rs Outdated Show resolved Hide resolved
chain/network/src/store/schema/mod.rs Show resolved Hide resolved
chain/network/src/store/schema/mod.rs Show resolved Hide resolved
pub struct AccountAnnouncements;
impl Column for AccountAnnouncements {
const COL: DBCol = DBCol::AccountAnnouncements;
type Key = AccountIdFormat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice !

/// methods writing to the DB.
pub struct Store(schema::Store);

impl Store {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider calling it NetworkStore (makes it a lot easier to read - and less confusion with 'main' Store)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NetworkStore is stuttering with the crate name. near_store::Store shouldn't be visible wherever store::Store is used (except for the moment of wrapping, but that's unavoidable).

chain/network/src/store/mod.rs Outdated Show resolved Hide resolved
chain/network/src/store/mod.rs Show resolved Hide resolved
@pompon0 pompon0 force-pushed the gprusak-store branch 2 times, most recently from 7fd31e3 to 05186d4 Compare June 10, 2022 12:37
@pompon0 pompon0 requested a review from matklad June 10, 2022 12:37
Base automatically changed from gprusak-time to master June 10, 2022 13:04
@pompon0 pompon0 force-pushed the gprusak-store branch 4 times, most recently from cffc173 to d788d6f Compare June 10, 2022 14:59
@pompon0 pompon0 self-assigned this Jun 10, 2022
@near-bulldozer near-bulldozer bot merged commit 2f89227 into master Jun 10, 2022
@near-bulldozer near-bulldozer bot deleted the gprusak-store branch June 10, 2022 15:10
nikurt pushed a commit that referenced this pull request Jun 13, 2022
It consists of 2 layers:
1. schema definition layer - enforcing type-safe access to the DB columns
2. atomic access layer - defining all DB transactions, which are the high level operations that business logic can execute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants