-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add ready time and priority to mempool messages for better metric calculation #14037
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
config/src/config/mempool_config.rs
Outdated
|
||
/// Uses the MempoolSyncMessageV2 instead of MempoolSyncMessage when sending mempool transactions | ||
/// to upstream nodes. | ||
pub use_mempool_sync_message_v2: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we call the new message type BroadcastTransactionsRequestV2
-> BroadcastTransactionsRequestWithReadyTime
or something similar that has significance (since we're not going to deprecate "v1"). Maybe this can be include_ready_time_in_broadcast
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -435,7 +435,7 @@ impl Default for AptosDataClientConfig { | |||
data_multi_fetch_config: AptosDataMultiFetchConfig::default(), | |||
ignore_low_score_peers: true, | |||
latency_filtering_config: AptosLatencyFilteringConfig::default(), | |||
latency_monitor_loop_interval_ms: 100, | |||
latency_monitor_loop_interval_ms: 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this change? Is this required in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually meant to improve the accuracy of state sync metric collection. Not relevant to this particular PR. So, removing it.
mempool/src/core_mempool/mempool.rs
Outdated
let status = self.transactions.insert(txn_info); | ||
let now = now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use aptos_infallible::duration_since_epoch().as_millis()
to be consistent with the rest of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
mempool/src/core_mempool/mempool.rs
Outdated
.as_millis() as u64; | ||
|
||
// TODO: Remove this before landing | ||
info!("txn added to mempool: {} {} status {}, priority {:?}, client_submitted {}, now: {:?}, inserted_at_sender {:?}, time_since: {:?}", txn.sender(), txn.sequence_number(), status, priority.clone(), client_submitted, now, ready_time_at_sender, Duration::from_millis(now.saturating_sub(ready_time_at_sender.unwrap_or(0)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure to remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
transactions: Vec<SignedTransaction>, | ||
// For each transaction, we include the ready time in millis since epoch | ||
transactions: Vec<(SignedTransaction, u64)>, | ||
use_mempool_sync_message_v2: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to pass this? We should be able to read from self.mempool_config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -592,13 +592,14 @@ impl TransactionStore { | |||
if batch_total_bytes.saturating_add(transaction_bytes) > self.max_batch_bytes { | |||
break; // The batch is full | |||
} else { | |||
batch.push((txn.txn.clone(), txn.insertion_info.insertion_time.duration_since(UNIX_EPOCH).expect("Failed to determine absolute unix time based on given duration") | |||
batch.push((txn.txn.clone(), txn.insertion_info.ready_time.duration_since(UNIX_EPOCH).expect("Failed to determine absolute unix time based on given duration") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use aptos_infallable::duration_since_epoch_at(txn.insertion_info.ready_time)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -640,7 +641,7 @@ impl TransactionStore { | |||
self.transactions | |||
.get(account) | |||
.and_then(|txns| txns.get(sequence_number)) | |||
.map(|txn| (txn.txn.clone(), txn.insertion_info.insertion_time.duration_since(UNIX_EPOCH).expect("Failed to determine absolute unix time based on given duration") | |||
.map(|txn| (txn.txn.clone(), txn.insertion_info.ready_time.duration_since(UNIX_EPOCH).expect("Failed to determine absolute unix time based on given duration") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
config/src/config/mempool_config.rs
Outdated
@@ -60,9 +60,9 @@ pub struct MempoolConfig { | |||
pub broadcast_buckets: Vec<u64>, | |||
pub eager_expire_threshold_ms: Option<u64>, | |||
pub eager_expire_time_ms: u64, | |||
/// Uses the MempoolSyncMessageV2 instead of MempoolSyncMessage when sending mempool transactions | |||
/// Uses the BroadcastTransactionsRequestWithReadyTime instead of MempoolSyncMessage when sending mempool transactions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove MempoolSyncMessage
-> BroadcastTransactionsRequestWithReadyTime instead of BroadcastTransactionsRequest
testsuite/forge-cli/src/main.rs
Outdated
@@ -603,11 +603,12 @@ fn k8s_test_suite() -> ForgeConfig { | |||
fn get_land_blocking_test( | |||
test_name: &str, | |||
duration: Duration, | |||
test_cmd: &TestCommand, | |||
_test_cmd: &TestCommand, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to revert the forge changes before landing
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist