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

refactor(chain): expunge rocksdb from near-jsonrpc-primitives dependency tree #4651

Merged
merged 12 commits into from
Aug 20, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions chain/chunks-primitives/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "near-chunks-primitives"
version = "0.1.0"
authors = ["Near Inc <hello@nearprotocol.com>"]
edition = "2018"

[dependencies]
near-chain-primitives = { path = "../chain-primitives" }
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::fmt;

#[derive(Debug)]
pub enum Error {
InvalidPartMessage,
Expand All @@ -10,12 +12,12 @@ pub enum Error {
DuplicateChunkHeight,
UnknownChunk,
KnownPart,
ChainError(near_chain::Error),
ChainError(near_chain_primitives::Error),
IOError(std::io::Error),
}

impl std::fmt::Display for Error {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
write!(f, "{:?}", self)
}
}
Expand All @@ -26,8 +28,8 @@ impl From<std::io::Error> for Error {
}
}

impl From<near_chain::Error> for Error {
fn from(err: near_chain::Error) -> Self {
impl From<near_chain_primitives::Error> for Error {
fn from(err: near_chain_primitives::Error) -> Self {
Error::ChainError(err)
}
}
3 changes: 3 additions & 0 deletions chain/chunks-primitives/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
mod error;

pub use error::Error;
1 change: 1 addition & 0 deletions chain/chunks/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ reed-solomon-erasure = "4"

near-crypto = { path = "../../core/crypto" }
near-primitives = { path = "../../core/primitives" }
near-chunks-primitives = { path = "../chunks-primitives" }
near-store = { path = "../../core/store" }
near-network = { path = "../network" }
near-chain = { path = "../chain" }
Expand Down
3 changes: 1 addition & 2 deletions chain/chunks/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,11 @@ use near_primitives::version::ProtocolVersion;
use near_primitives::{checked_feature, unwrap_or_return};

use crate::chunk_cache::{EncodedChunksCache, EncodedChunksCacheEntry};
pub use crate::types::Error;
pub use near_chunks_primitives::Error;
use rand::Rng;

mod chunk_cache;
pub mod test_utils;
mod types;

const CHUNK_PRODUCER_BLACKLIST_SIZE: usize = 100;
pub const CHUNK_REQUEST_RETRY_MS: u64 = 100;
Expand Down
6 changes: 3 additions & 3 deletions chain/client-primitives/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ thiserror = "1.0"
near-chain-primitives = { path = "../chain-primitives" }
near-chain-configs = { path = "../../core/chain-configs" }

near-chunks = { path = "../chunks" }
near-chunks-primitives = { path = "../chunks-primitives" }
near-crypto = { path = "../../core/crypto" }
near-network = { path = "../network" }
near-network-primitives = { path = "../network-primitives" }
near-primitives = { path = "../../core/primitives" }

[features]
metric_recorder = []
metric_recorder = ["near-network-primitives/metric_recorder"]
11 changes: 5 additions & 6 deletions chain/client-primitives/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#[cfg(feature = "metric_recorder")]
use near_network::recorder::MetricRecorder;
use near_network_primitives::recorder::MetricRecorder;
use std::collections::HashMap;
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::Arc;
Expand All @@ -9,8 +9,7 @@ use chrono::{DateTime, Utc};
use serde::{Deserialize, Serialize};

use near_chain_configs::ProtocolConfigView;
use near_network::types::{AccountOrPeerIdOrHash, KnownProducer};
use near_network::PeerInfo;
use near_network_primitives::types::{AccountOrPeerIdOrHash, KnownProducer, PeerInfo};
use near_primitives::errors::InvalidTxError;
use near_primitives::hash::CryptoHash;
use near_primitives::merkle::{MerklePath, PartialMerkleTree};
Expand All @@ -33,7 +32,7 @@ pub use near_primitives::views::{StatusResponse, StatusSyncInfo};
#[derive(Debug)]
pub enum Error {
Chain(near_chain_primitives::Error),
Chunk(near_chunks::Error),
Chunk(near_chunks_primitives::Error),
BlockProducer(String),
ChunkProducer(String),
Other(String),
Expand Down Expand Up @@ -66,8 +65,8 @@ impl From<near_chain_primitives::ErrorKind> for Error {
}
}

impl From<near_chunks::Error> for Error {
fn from(err: near_chunks::Error) -> Self {
impl From<near_chunks_primitives::Error> for Error {
fn from(err: near_chunks_primitives::Error) -> Self {
Error::Chunk(err)
}
}
Expand Down
2 changes: 1 addition & 1 deletion chain/client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ near-test-contracts = { path = "../../runtime/near-test-contracts" }
byzantine_asserts = ["near-chain/byzantine_asserts"]
expensive_tests = []
adversarial = ["near-network/adversarial", "near-chain/adversarial"]
metric_recorder = ["near-client-primitives/metric_recorder"]
metric_recorder = ["near-network/metric_recorder", "near-client-primitives/metric_recorder"]
delay_detector = ["near-chain/delay_detector", "near-network/delay_detector", "delay-detector"]
protocol_feature_block_header_v3 = ["near-primitives/protocol_feature_block_header_v3", "near-chain/protocol_feature_block_header_v3", "near-store/protocol_feature_block_header_v3"]
protocol_feature_restore_receipts_after_fix = []
Expand Down
28 changes: 28 additions & 0 deletions chain/network-primitives/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
[package]
name = "near-network-primitives"
version = "0.1.0"
authors = ["Near Inc <hello@nearprotocol.com>"]
edition = "2018"

[dependencies]
actix = "=0.11.0-beta.2"
tokio = { version = "1.1", features = ["full"] }
futures = "0.3"
chrono = { version = "0.4.4", features = ["serde"] }
borsh = "0.8.1"
serde = { version = "1", features = [ "derive" ] }
serde_json = "1"
strum = { version = "0.20", features = ["derive"] }
tracing = "0.1.13"
conqueue = "0.4.0"
byteorder = "1.2"
lazy_static = "1.4"

near-crypto = { path = "../../core/crypto" }
near-primitives = { path = "../../core/primitives" }
near-metrics = { path = "../../core/metrics" }

[features]
adversarial = []
metric_recorder = []
sandbox = []
6 changes: 6 additions & 0 deletions chain/network-primitives/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#[cfg(feature = "metric_recorder")]
pub mod recorder;

pub mod metrics;
pub mod routing;
pub mod types;
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ use near_metrics::{
use std::collections::HashMap;
use strum::VariantNames;

use lazy_static::lazy_static;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for moving this file to network-primitives? It feels to me that those counters should be defined in the network crate itself

Copy link
Contributor Author

@miraclx miraclx Aug 11, 2021

Choose a reason for hiding this comment

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

You're right. Nice catch!


What are the criteria for deciding what goes into network-primitives?

In this case, we just initialize the crate and move whatever's necessary to liberate near-client-primitives from the excess dependencies.
Going forward, we can incrementally move whatever's appropriate into their equivalent -primitives crates.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the dependency on Edge come from? I don't think it is used outside of the network crate

Copy link
Contributor Author

@miraclx miraclx Aug 12, 2021

Choose a reason for hiding this comment

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

So, while near-client-primitives only required;

near_network::{recorder::MetricRecorder, types::{AccountOrPeerIdOrHash, KnownProducer}, PeerInfo}

In the case of near_network::types, I believed it was safe to extract as much as was possible since that module name typically encapsulates reusable primitives. And that worked fine with the only exception being Consolidate which is retained in near_network::types.

Following that logic, routing::Edge has a number of dependents in near_network_primitives::types. So, naturally, it's extracted alongside it.


That aside, recorder::MetricRecorder depends on types::PeerMessage which depends on types::Edge

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believed it was safe to extract as much as was possible since that module name typically encapsulates reusable primitives

Well that is exactly my concern. Edge should not be used outside of the network crate. Why do we need MetricRecorder in near-client-primitives?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. @miraclx please remove metric_recorder from NetworkInfoResponse

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mfornet @bowenwang1996 should the whole feature be removed altogether or just drop the field from the NetworkInfoResponse? Is the feature still useful without the exposed data?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mfornet @bowenwang1996 should the whole feature be removed altogether or just drop the field from the NetworkInfoResponse? Is the feature still useful without the exposed data?

I think the feature should be removed altogether, but my point is that we can now revert the change that extracts network primitives since I don't think they should be in a separate crate to begin with @miraclx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MetricRecorder was removed, which freed up the dependency on PeerMessage which, in-turn, freed up the dependency on Edge. This means ::routing and a number of structures in ::types we're returned to near-network.
But we (near_client_primitives) still need near_network_primitives::types::{AccountOrPeerIdOrHash, KnownProducer, PeerInfo}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bowenwang1996 @mfornet Could you review the PR after the update?


lazy_static! {
pub static ref PEER_CONNECTIONS_TOTAL: near_metrics::Result<IntGauge> =
try_create_int_gauge("near_peer_connections_total", "Number of connected peers");
Expand Down
File renamed without changes.
Loading