refactor: store ShardId in child nodes

Instead of passing the ShardId into each function for child nodes of the
Shard, store it. This avoids the possibility of mistakenly passing the
wrong value.
pull/24376/head
Dom Dwyer 2022-09-16 17:52:38 +02:00
parent 07b08fa9cb
commit b0eb85ddd5
7 changed files with 72 additions and 43 deletions

View File

@ -166,7 +166,6 @@ impl IngesterData {
shard_data shard_data
.buffer_operation( .buffer_operation(
dml_operation, dml_operation,
shard_id,
self.catalog.as_ref(), self.catalog.as_ref(),
lifecycle_handle, lifecycle_handle,
&self.exec, &self.exec,
@ -653,7 +652,10 @@ mod tests {
let mut shards = BTreeMap::new(); let mut shards = BTreeMap::new();
let shard_index = ShardIndex::new(0); let shard_index = ShardIndex::new(0);
shards.insert(shard1.id, ShardData::new(shard_index, Arc::clone(&metrics))); shards.insert(
shard1.id,
ShardData::new(shard_index, shard1.id, Arc::clone(&metrics)),
);
let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new()); let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new());
@ -738,7 +740,7 @@ mod tests {
let mut shards = BTreeMap::new(); let mut shards = BTreeMap::new();
shards.insert( shards.insert(
shard1.id, shard1.id,
ShardData::new(shard1.shard_index, Arc::clone(&metrics)), ShardData::new(shard1.shard_index, shard1.id, Arc::clone(&metrics)),
); );
let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new()); let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new());
@ -843,11 +845,11 @@ mod tests {
let mut shards = BTreeMap::new(); let mut shards = BTreeMap::new();
shards.insert( shards.insert(
shard1.id, shard1.id,
ShardData::new(shard1.shard_index, Arc::clone(&metrics)), ShardData::new(shard1.shard_index, shard1.id, Arc::clone(&metrics)),
); );
shards.insert( shards.insert(
shard2.id, shard2.id,
ShardData::new(shard2.shard_index, Arc::clone(&metrics)), ShardData::new(shard2.shard_index, shard2.id, Arc::clone(&metrics)),
); );
let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new()); let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new());
@ -1099,11 +1101,11 @@ mod tests {
let mut shards = BTreeMap::new(); let mut shards = BTreeMap::new();
shards.insert( shards.insert(
shard1.id, shard1.id,
ShardData::new(shard1.shard_index, Arc::clone(&metrics)), ShardData::new(shard1.shard_index, shard1.id, Arc::clone(&metrics)),
); );
shards.insert( shards.insert(
shard2.id, shard2.id,
ShardData::new(shard2.shard_index, Arc::clone(&metrics)), ShardData::new(shard2.shard_index, shard2.id, Arc::clone(&metrics)),
); );
let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new()); let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new());
@ -1338,7 +1340,7 @@ mod tests {
); );
let exec = Executor::new(1); let exec = Executor::new(1);
let data = NamespaceData::new(namespace.id, &*metrics); let data = NamespaceData::new(namespace.id, shard.id, &*metrics);
// w1 should be ignored because the per-partition replay offset is set // w1 should be ignored because the per-partition replay offset is set
// to 1 already, so it shouldn't be buffered and the buffer should // to 1 already, so it shouldn't be buffered and the buffer should
@ -1346,7 +1348,6 @@ mod tests {
let should_pause = data let should_pause = data
.buffer_operation( .buffer_operation(
DmlOperation::Write(w1), DmlOperation::Write(w1),
shard.id,
catalog.as_ref(), catalog.as_ref(),
&manager.handle(), &manager.handle(),
&exec, &exec,
@ -1368,7 +1369,6 @@ mod tests {
// w2 should be in the buffer // w2 should be in the buffer
data.buffer_operation( data.buffer_operation(
DmlOperation::Write(w2), DmlOperation::Write(w2),
shard.id,
catalog.as_ref(), catalog.as_ref(),
&manager.handle(), &manager.handle(),
&exec, &exec,
@ -1410,7 +1410,10 @@ mod tests {
let mut shards = BTreeMap::new(); let mut shards = BTreeMap::new();
let shard_index = ShardIndex::new(0); let shard_index = ShardIndex::new(0);
shards.insert(shard1.id, ShardData::new(shard_index, Arc::clone(&metrics))); shards.insert(
shard1.id,
ShardData::new(shard_index, shard1.id, Arc::clone(&metrics)),
);
let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new()); let object_store: Arc<DynObjectStore> = Arc::new(InMemory::new());

View File

@ -26,8 +26,11 @@ use crate::lifecycle::LifecycleHandle;
#[derive(Debug)] #[derive(Debug)]
pub struct NamespaceData { pub struct NamespaceData {
namespace_id: NamespaceId, namespace_id: NamespaceId,
tables: RwLock<BTreeMap<String, Arc<tokio::sync::RwLock<TableData>>>>,
/// The catalog ID of the shard this namespace is being populated from.
shard_id: ShardId,
tables: RwLock<BTreeMap<String, Arc<tokio::sync::RwLock<TableData>>>>,
table_count: U64Counter, table_count: U64Counter,
/// The sequence number being actively written, if any. /// The sequence number being actively written, if any.
@ -78,7 +81,7 @@ pub struct NamespaceData {
impl NamespaceData { impl NamespaceData {
/// Initialize new tables with default partition template of daily /// Initialize new tables with default partition template of daily
pub fn new(namespace_id: NamespaceId, metrics: &metric::Registry) -> Self { pub fn new(namespace_id: NamespaceId, shard_id: ShardId, metrics: &metric::Registry) -> Self {
let table_count = metrics let table_count = metrics
.register_metric::<U64Counter>( .register_metric::<U64Counter>(
"ingester_tables_total", "ingester_tables_total",
@ -88,6 +91,7 @@ impl NamespaceData {
Self { Self {
namespace_id, namespace_id,
shard_id,
tables: Default::default(), tables: Default::default(),
table_count, table_count,
buffering_sequence_number: RwLock::new(None), buffering_sequence_number: RwLock::new(None),
@ -100,10 +104,12 @@ impl NamespaceData {
#[cfg(test)] #[cfg(test)]
pub(crate) fn new_for_test( pub(crate) fn new_for_test(
namespace_id: NamespaceId, namespace_id: NamespaceId,
shard_id: ShardId,
tables: BTreeMap<String, Arc<tokio::sync::RwLock<TableData>>>, tables: BTreeMap<String, Arc<tokio::sync::RwLock<TableData>>>,
) -> Self { ) -> Self {
Self { Self {
namespace_id, namespace_id,
shard_id,
tables: RwLock::new(tables), tables: RwLock::new(tables),
table_count: Default::default(), table_count: Default::default(),
buffering_sequence_number: RwLock::new(None), buffering_sequence_number: RwLock::new(None),
@ -117,7 +123,6 @@ impl NamespaceData {
pub async fn buffer_operation( pub async fn buffer_operation(
&self, &self,
dml_operation: DmlOperation, dml_operation: DmlOperation,
shard_id: ShardId,
catalog: &dyn Catalog, catalog: &dyn Catalog,
lifecycle_handle: &dyn LifecycleHandle, lifecycle_handle: &dyn LifecycleHandle,
executor: &Executor, executor: &Executor,
@ -148,7 +153,7 @@ impl NamespaceData {
for (t, b) in write.into_tables() { for (t, b) in write.into_tables() {
let table_data = match self.table_data(&t) { let table_data = match self.table_data(&t) {
Some(t) => t, Some(t) => t,
None => self.insert_table(shard_id, &t, catalog).await?, None => self.insert_table(&t, catalog).await?,
}; };
{ {
@ -159,7 +164,6 @@ impl NamespaceData {
sequence_number, sequence_number,
b, b,
partition_key.clone(), partition_key.clone(),
shard_id,
catalog, catalog,
lifecycle_handle, lifecycle_handle,
) )
@ -176,19 +180,13 @@ impl NamespaceData {
let table_name = delete.table_name().context(super::TableNotPresentSnafu)?; let table_name = delete.table_name().context(super::TableNotPresentSnafu)?;
let table_data = match self.table_data(table_name) { let table_data = match self.table_data(table_name) {
Some(t) => t, Some(t) => t,
None => self.insert_table(shard_id, table_name, catalog).await?, None => self.insert_table(table_name, catalog).await?,
}; };
let mut table_data = table_data.write().await; let mut table_data = table_data.write().await;
table_data table_data
.buffer_delete( .buffer_delete(delete.predicate(), sequence_number, catalog, executor)
delete.predicate(),
shard_id,
sequence_number,
catalog,
executor,
)
.await?; .await?;
// don't pause writes since deletes don't count towards memory limits // don't pause writes since deletes don't count towards memory limits
@ -250,14 +248,13 @@ impl NamespaceData {
/// Inserts the table or returns it if it happens to be inserted by some other thread /// Inserts the table or returns it if it happens to be inserted by some other thread
async fn insert_table( async fn insert_table(
&self, &self,
shard_id: ShardId,
table_name: &str, table_name: &str,
catalog: &dyn Catalog, catalog: &dyn Catalog,
) -> Result<Arc<tokio::sync::RwLock<TableData>>, super::Error> { ) -> Result<Arc<tokio::sync::RwLock<TableData>>, super::Error> {
let mut repos = catalog.repositories().await; let mut repos = catalog.repositories().await;
let info = repos let info = repos
.tables() .tables()
.get_table_persist_info(shard_id, self.namespace_id, table_name) .get_table_persist_info(self.shard_id, self.namespace_id, table_name)
.await .await
.context(super::CatalogSnafu)? .context(super::CatalogSnafu)?
.context(super::TableNotFoundSnafu { table_name })?; .context(super::TableNotFoundSnafu { table_name })?;
@ -269,6 +266,7 @@ impl NamespaceData {
let v = v.insert(Arc::new(tokio::sync::RwLock::new(TableData::new( let v = v.insert(Arc::new(tokio::sync::RwLock::new(TableData::new(
info.table_id, info.table_id,
table_name, table_name,
self.shard_id,
info.tombstone_max_sequence_number, info.tombstone_max_sequence_number,
)))); ))));
self.table_count.inc(1); self.table_count.inc(1);

View File

@ -22,6 +22,8 @@ use crate::lifecycle::LifecycleHandle;
pub struct ShardData { pub struct ShardData {
/// The shard index for this shard /// The shard index for this shard
shard_index: ShardIndex, shard_index: ShardIndex,
/// The catalog ID for this shard.
shard_id: ShardId,
// New namespaces can come in at any time so we need to be able to add new ones // New namespaces can come in at any time so we need to be able to add new ones
namespaces: RwLock<BTreeMap<String, Arc<NamespaceData>>>, namespaces: RwLock<BTreeMap<String, Arc<NamespaceData>>>,
@ -32,7 +34,7 @@ pub struct ShardData {
impl ShardData { impl ShardData {
/// Initialise a new [`ShardData`] that emits metrics to `metrics`. /// Initialise a new [`ShardData`] that emits metrics to `metrics`.
pub fn new(shard_index: ShardIndex, metrics: Arc<metric::Registry>) -> Self { pub fn new(shard_index: ShardIndex, shard_id: ShardId, metrics: Arc<metric::Registry>) -> Self {
let namespace_count = metrics let namespace_count = metrics
.register_metric::<U64Counter>( .register_metric::<U64Counter>(
"ingester_namespaces_total", "ingester_namespaces_total",
@ -42,6 +44,7 @@ impl ShardData {
Self { Self {
shard_index, shard_index,
shard_id,
namespaces: Default::default(), namespaces: Default::default(),
metrics, metrics,
namespace_count, namespace_count,
@ -52,10 +55,12 @@ impl ShardData {
#[cfg(test)] #[cfg(test)]
pub fn new_for_test( pub fn new_for_test(
shard_index: ShardIndex, shard_index: ShardIndex,
shard_id: ShardId,
namespaces: BTreeMap<String, Arc<NamespaceData>>, namespaces: BTreeMap<String, Arc<NamespaceData>>,
) -> Self { ) -> Self {
Self { Self {
shard_index, shard_index,
shard_id,
namespaces: RwLock::new(namespaces), namespaces: RwLock::new(namespaces),
metrics: Default::default(), metrics: Default::default(),
namespace_count: Default::default(), namespace_count: Default::default(),
@ -69,7 +74,6 @@ impl ShardData {
pub async fn buffer_operation( pub async fn buffer_operation(
&self, &self,
dml_operation: DmlOperation, dml_operation: DmlOperation,
shard_id: ShardId,
catalog: &dyn Catalog, catalog: &dyn Catalog,
lifecycle_handle: &dyn LifecycleHandle, lifecycle_handle: &dyn LifecycleHandle,
executor: &Executor, executor: &Executor,
@ -83,7 +87,7 @@ impl ShardData {
}; };
namespace_data namespace_data
.buffer_operation(dml_operation, shard_id, catalog, lifecycle_handle, executor) .buffer_operation(dml_operation, catalog, lifecycle_handle, executor)
.await .await
} }
@ -112,7 +116,11 @@ impl ShardData {
let data = match n.entry(namespace.name) { let data = match n.entry(namespace.name) {
Entry::Vacant(v) => { Entry::Vacant(v) => {
let v = v.insert(Arc::new(NamespaceData::new(namespace.id, &*self.metrics))); let v = v.insert(Arc::new(NamespaceData::new(
namespace.id,
self.shard_id,
&*self.metrics,
)));
self.namespace_count.inc(1); self.namespace_count.inc(1);
Arc::clone(v) Arc::clone(v)
} }

View File

@ -17,6 +17,10 @@ use crate::lifecycle::LifecycleHandle;
pub(crate) struct TableData { pub(crate) struct TableData {
table_id: TableId, table_id: TableId,
table_name: Arc<str>, table_name: Arc<str>,
/// The catalog ID of the shard this table is being populated from.
shard_id: ShardId,
// the max sequence number for a tombstone associated with this table // the max sequence number for a tombstone associated with this table
tombstone_max_sequence_number: Option<SequenceNumber>, tombstone_max_sequence_number: Option<SequenceNumber>,
// Map pf partition key to its data // Map pf partition key to its data
@ -28,11 +32,13 @@ impl TableData {
pub fn new( pub fn new(
table_id: TableId, table_id: TableId,
table_name: &str, table_name: &str,
shard_id: ShardId,
tombstone_max_sequence_number: Option<SequenceNumber>, tombstone_max_sequence_number: Option<SequenceNumber>,
) -> Self { ) -> Self {
Self { Self {
table_id, table_id,
table_name: table_name.into(), table_name: table_name.into(),
shard_id,
tombstone_max_sequence_number, tombstone_max_sequence_number,
partition_data: Default::default(), partition_data: Default::default(),
} }
@ -43,12 +49,14 @@ impl TableData {
pub fn new_for_test( pub fn new_for_test(
table_id: TableId, table_id: TableId,
table_name: &str, table_name: &str,
shard_id: ShardId,
tombstone_max_sequence_number: Option<SequenceNumber>, tombstone_max_sequence_number: Option<SequenceNumber>,
partitions: BTreeMap<PartitionKey, PartitionData>, partitions: BTreeMap<PartitionKey, PartitionData>,
) -> Self { ) -> Self {
Self { Self {
table_id, table_id,
table_name: table_name.into(), table_name: table_name.into(),
shard_id,
tombstone_max_sequence_number, tombstone_max_sequence_number,
partition_data: partitions, partition_data: partitions,
} }
@ -76,14 +84,13 @@ impl TableData {
sequence_number: SequenceNumber, sequence_number: SequenceNumber,
batch: MutableBatch, batch: MutableBatch,
partition_key: PartitionKey, partition_key: PartitionKey,
shard_id: ShardId,
catalog: &dyn Catalog, catalog: &dyn Catalog,
lifecycle_handle: &dyn LifecycleHandle, lifecycle_handle: &dyn LifecycleHandle,
) -> Result<bool, super::Error> { ) -> Result<bool, super::Error> {
let partition_data = match self.partition_data.get_mut(&partition_key) { let partition_data = match self.partition_data.get_mut(&partition_key) {
Some(p) => p, Some(p) => p,
None => { None => {
self.insert_partition(partition_key.clone(), shard_id, catalog) self.insert_partition(partition_key.clone(), self.shard_id, catalog)
.await?; .await?;
self.partition_data.get_mut(&partition_key).unwrap() self.partition_data.get_mut(&partition_key).unwrap()
} }
@ -98,7 +105,7 @@ impl TableData {
let should_pause = lifecycle_handle.log_write( let should_pause = lifecycle_handle.log_write(
partition_data.id(), partition_data.id(),
shard_id, self.shard_id,
sequence_number, sequence_number,
batch.size(), batch.size(),
batch.rows(), batch.rows(),
@ -111,7 +118,6 @@ impl TableData {
pub(super) async fn buffer_delete( pub(super) async fn buffer_delete(
&mut self, &mut self,
predicate: &DeletePredicate, predicate: &DeletePredicate,
shard_id: ShardId,
sequence_number: SequenceNumber, sequence_number: SequenceNumber,
catalog: &dyn Catalog, catalog: &dyn Catalog,
executor: &Executor, executor: &Executor,
@ -124,7 +130,7 @@ impl TableData {
.tombstones() .tombstones()
.create_or_get( .create_or_get(
self.table_id, self.table_id,
shard_id, self.shard_id,
sequence_number, sequence_number,
min_time, min_time,
max_time, max_time,

View File

@ -140,7 +140,7 @@ impl IngestHandlerImpl {
for s in shard_states.values() { for s in shard_states.values() {
shards.insert( shards.insert(
s.id, s.id,
ShardData::new(s.shard_index, Arc::clone(&metric_registry)), ShardData::new(s.shard_index, s.id, Arc::clone(&metric_registry)),
); );
} }
let data = Arc::new(IngesterData::new( let data = Arc::new(IngesterData::new(

View File

@ -692,11 +692,13 @@ pub fn make_ingester_data(two_partitions: bool, loc: DataLocation) -> IngesterDa
let empty_tbl = Arc::new(tokio::sync::RwLock::new(TableData::new( let empty_tbl = Arc::new(tokio::sync::RwLock::new(TableData::new(
empty_table_id, empty_table_id,
"test_table", "test_table",
shard_id,
None, None,
))); )));
let data_tbl = Arc::new(tokio::sync::RwLock::new(TableData::new_for_test( let data_tbl = Arc::new(tokio::sync::RwLock::new(TableData::new_for_test(
data_table_id, data_table_id,
"test_table", "test_table",
shard_id,
None, None,
partitions, partitions,
))); )));
@ -705,14 +707,18 @@ pub fn make_ingester_data(two_partitions: bool, loc: DataLocation) -> IngesterDa
// Two namespaces: one empty and one with data of 2 tables // Two namespaces: one empty and one with data of 2 tables
let mut namespaces = BTreeMap::new(); let mut namespaces = BTreeMap::new();
let empty_ns = Arc::new(NamespaceData::new(NamespaceId::new(1), &*metrics)); let empty_ns = Arc::new(NamespaceData::new(NamespaceId::new(1), shard_id, &*metrics));
let data_ns = Arc::new(NamespaceData::new_for_test(NamespaceId::new(2), tables)); let data_ns = Arc::new(NamespaceData::new_for_test(
NamespaceId::new(2),
shard_id,
tables,
));
namespaces.insert(TEST_NAMESPACE_EMPTY.to_string(), empty_ns); namespaces.insert(TEST_NAMESPACE_EMPTY.to_string(), empty_ns);
namespaces.insert(TEST_NAMESPACE.to_string(), data_ns); namespaces.insert(TEST_NAMESPACE.to_string(), data_ns);
// One shard that contains 2 namespaces // One shard that contains 2 namespaces
let shard_index = ShardIndex::new(0); let shard_index = ShardIndex::new(0);
let shard_data = ShardData::new_for_test(shard_index, namespaces); let shard_data = ShardData::new_for_test(shard_index, shard_id, namespaces);
let mut shards = BTreeMap::new(); let mut shards = BTreeMap::new();
shards.insert(shard_id, shard_data); shards.insert(shard_id, shard_data);
@ -743,7 +749,7 @@ pub async fn make_ingester_data_with_tombstones(loc: DataLocation) -> IngesterDa
// Two tables: one empty and one with data of one or two partitions // Two tables: one empty and one with data of one or two partitions
let mut tables = BTreeMap::new(); let mut tables = BTreeMap::new();
let data_tbl = TableData::new_for_test(data_table_id, TEST_TABLE, None, partitions); let data_tbl = TableData::new_for_test(data_table_id, TEST_TABLE, shard_id, None, partitions);
tables.insert( tables.insert(
TEST_TABLE.to_string(), TEST_TABLE.to_string(),
Arc::new(tokio::sync::RwLock::new(data_tbl)), Arc::new(tokio::sync::RwLock::new(data_tbl)),
@ -751,12 +757,16 @@ pub async fn make_ingester_data_with_tombstones(loc: DataLocation) -> IngesterDa
// Two namespaces: one empty and one with data of 2 tables // Two namespaces: one empty and one with data of 2 tables
let mut namespaces = BTreeMap::new(); let mut namespaces = BTreeMap::new();
let data_ns = Arc::new(NamespaceData::new_for_test(NamespaceId::new(2), tables)); let data_ns = Arc::new(NamespaceData::new_for_test(
NamespaceId::new(2),
shard_id,
tables,
));
namespaces.insert(TEST_NAMESPACE.to_string(), data_ns); namespaces.insert(TEST_NAMESPACE.to_string(), data_ns);
// One shard that contains 1 namespace // One shard that contains 1 namespace
let shard_index = ShardIndex::new(0); let shard_index = ShardIndex::new(0);
let shard_data = ShardData::new_for_test(shard_index, namespaces); let shard_data = ShardData::new_for_test(shard_index, shard_id, namespaces);
let mut shards = BTreeMap::new(); let mut shards = BTreeMap::new();
shards.insert(shard_id, shard_data); shards.insert(shard_id, shard_data);

View File

@ -694,7 +694,11 @@ impl MockIngester {
let shards = BTreeMap::from([( let shards = BTreeMap::from([(
shard.shard.id, shard.shard.id,
ShardData::new(shard.shard.shard_index, catalog.metric_registry()), ShardData::new(
shard.shard.shard_index,
shard.shard.id,
catalog.metric_registry(),
),
)]); )]);
let ingester_data = Arc::new(IngesterData::new( let ingester_data = Arc::new(IngesterData::new(
catalog.object_store(), catalog.object_store(),