| Message ID | 404d64effde3bee424e338d858ed507ff83dff20.1723528765.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: drop generic `reftable_table` interface | expand |
Patrick Steinhardt <ps@pks.im> writes: > We do not expose any functions via our public headers that would allow a > caller to initialize a reftable iterator from a merged table. Instead, > they are expected to go via the generic `reftable_table` interface, > which is somewhat roundabout. > > Implement two new functions to initialize iterators for ref and log > records to plug this gap. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/merged.c | 12 ++++++++++++ > reftable/reftable-merged.h | 8 ++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/reftable/merged.c b/reftable/merged.c > index 6adce44f4b..8d78b3da71 100644 > --- a/reftable/merged.c > +++ b/reftable/merged.c > @@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt, > iterator_from_merged_iter(it, mi); > } > > +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt, > + struct reftable_iterator *it) > +{ > + merged_table_init_iter(mt, it, BLOCK_TYPE_REF); > +} > + > +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt, > + struct reftable_iterator *it) > +{ > + merged_table_init_iter(mt, it, BLOCK_TYPE_LOG); > +} > + These too look similar to `static void reftable_merged_table_init_iter_void` defined a little below, I wonder if we could have simply exposed that? Perhaps we remove it in the upcoming patches. [snip]
On Tue, Aug 13, 2024 at 05:36:27AM -0400, karthik nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > We do not expose any functions via our public headers that would allow a > > caller to initialize a reftable iterator from a merged table. Instead, > > they are expected to go via the generic `reftable_table` interface, > > which is somewhat roundabout. > > > > Implement two new functions to initialize iterators for ref and log > > records to plug this gap. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > reftable/merged.c | 12 ++++++++++++ > > reftable/reftable-merged.h | 8 ++++++++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/reftable/merged.c b/reftable/merged.c > > index 6adce44f4b..8d78b3da71 100644 > > --- a/reftable/merged.c > > +++ b/reftable/merged.c > > @@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt, > > iterator_from_merged_iter(it, mi); > > } > > > > +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt, > > + struct reftable_iterator *it) > > +{ > > + merged_table_init_iter(mt, it, BLOCK_TYPE_REF); > > +} > > + > > +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt, > > + struct reftable_iterator *it) > > +{ > > + merged_table_init_iter(mt, it, BLOCK_TYPE_LOG); > > +} > > + > > These too look similar to `static void > reftable_merged_table_init_iter_void` defined a little below, I wonder > if we could have simply exposed that? Perhaps we remove it in the > upcoming patches. Yup, `reftable_merged_table_init_iter_void()` is about to go away. But regardless of that, even if it didn't it would make sense to have both functions. The `iter_void()` one takes an arbitrary block type as input, which we never expose via our public interfaces. So it has to stay an implementation detail. The new ones added here do not expose the block type. Patrick
On 24/08/13 08:24AM, Patrick Steinhardt wrote: > We do not expose any functions via our public headers that would allow a > caller to initialize a reftable iterator from a merged table. Instead, > they are expected to go via the generic `reftable_table` interface, > which is somewhat roundabout. > > Implement two new functions to initialize iterators for ref and log > records to plug this gap. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- [snip] > diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h > index 14d5fc9f05..4deb0ad22e 100644 > --- a/reftable/reftable-merged.h > +++ b/reftable/reftable-merged.h > @@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest, > struct reftable_table *stack, size_t n, > uint32_t hash_id); > > +/* Initialize a merged table iterator for reading refs. */ > +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt, > + struct reftable_iterator *it); > + > +/* Initialize a merged table iterator for reading logs. */ > +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt, > + struct reftable_iterator *it); I might have missed it, but I do not see `reftable_merged_table_init_log_iterator()` used anywhere in the later patches. Does this need to be added? Or are we just adding it because we want a companion function to the ref iterator to be more consistent? -Justin
On Mon, Aug 19, 2024 at 11:55:16AM -0500, Justin Tobler wrote: > On 24/08/13 08:24AM, Patrick Steinhardt wrote: > > We do not expose any functions via our public headers that would allow a > > caller to initialize a reftable iterator from a merged table. Instead, > > they are expected to go via the generic `reftable_table` interface, > > which is somewhat roundabout. > > > > Implement two new functions to initialize iterators for ref and log > > records to plug this gap. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > [snip] > > diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h > > index 14d5fc9f05..4deb0ad22e 100644 > > --- a/reftable/reftable-merged.h > > +++ b/reftable/reftable-merged.h > > @@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest, > > struct reftable_table *stack, size_t n, > > uint32_t hash_id); > > > > +/* Initialize a merged table iterator for reading refs. */ > > +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt, > > + struct reftable_iterator *it); > > + > > +/* Initialize a merged table iterator for reading logs. */ > > +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt, > > + struct reftable_iterator *it); > > I might have missed it, but I do not see > `reftable_merged_table_init_log_iterator()` used anywhere in the later > patches. Does this need to be added? Or are we just adding it because we > want a companion function to the ref iterator to be more consistent? The latter. It really should exist to make the public interface complete. Patrick
diff --git a/reftable/merged.c b/reftable/merged.c index 6adce44f4b..8d78b3da71 100644 --- a/reftable/merged.c +++ b/reftable/merged.c @@ -254,6 +254,18 @@ void merged_table_init_iter(struct reftable_merged_table *mt, iterator_from_merged_iter(it, mi); } +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt, + struct reftable_iterator *it) +{ + merged_table_init_iter(mt, it, BLOCK_TYPE_REF); +} + +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt, + struct reftable_iterator *it) +{ + merged_table_init_iter(mt, it, BLOCK_TYPE_LOG); +} + uint32_t reftable_merged_table_hash_id(struct reftable_merged_table *mt) { return mt->hash_id; diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h index 14d5fc9f05..4deb0ad22e 100644 --- a/reftable/reftable-merged.h +++ b/reftable/reftable-merged.h @@ -36,6 +36,14 @@ int reftable_new_merged_table(struct reftable_merged_table **dest, struct reftable_table *stack, size_t n, uint32_t hash_id); +/* Initialize a merged table iterator for reading refs. */ +void reftable_merged_table_init_ref_iterator(struct reftable_merged_table *mt, + struct reftable_iterator *it); + +/* Initialize a merged table iterator for reading logs. */ +void reftable_merged_table_init_log_iterator(struct reftable_merged_table *mt, + struct reftable_iterator *it); + /* returns the max update_index covered by this merged table. */ uint64_t reftable_merged_table_max_update_index(struct reftable_merged_table *mt);
We do not expose any functions via our public headers that would allow a caller to initialize a reftable iterator from a merged table. Instead, they are expected to go via the generic `reftable_table` interface, which is somewhat roundabout. Implement two new functions to initialize iterators for ref and log records to plug this gap. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/merged.c | 12 ++++++++++++ reftable/reftable-merged.h | 8 ++++++++ 2 files changed, 20 insertions(+)