Feat[Storagetool]: Add searching by record sequence numbers and offsets #508

alexander-e1off · 2024-11-11T13:47:12Z

Enhance storage tool to search by composite sequence numbers (primaryLeaseId, sequenceNumber) and offsets:

Search up to a given sequence number or offset;
Search starting from a given sequence number or offset;
Search specific sequence number(s) or offsets;

Below are examples how to use new feature:

Filter messages within composite sequence numbers (primaryLeaseId, sequenceNumber) range:

bmqstoragetool --journal-file=<path> --seqnum-lt=<leaseId-sequenceNumber>
bmqstoragetool --journal-file=<path> --seqnum-gt=<leaseId-sequenceNumber>
bmqstoragetool --journal-file=<path> --seqnum-lt=<leaseId1-sequenceNumber1> --seqnum-gt=<leaseId2-sequenceNumber2>

Filter messages within record offsets range:

bmqstoragetool --journal-file=<path> --offset-lt=<offset>
bmqstoragetool --journal-file=<path> --offset-gt=<offset>
bmqstoragetool --journal-file=<path> --offset-lt=<offset1> --offset-gt=<offset2>

Filter messages with corresponding composite sequence numbers (defined in form primaryLeaseId-sequenceNumber):

bmqstoragetool --journal-file=<path> --seqnum=<leaseId-sequenceNumber_1> --seqnum=<leaseId-sequenceNumber_N>

Filter messages with corresponding record offsets:

bmqstoragetool --journal-file=<path> --offset=<offset_1> --offset=<offset_N>

waldgange · 2024-11-15T13:06:54Z

src/applications/bmqstoragetool/m_bmqstoragetool_journalfileprocessor.cpp

+template <typename T>
+T getValue(const mqbs::JournalFileIterator*  jit,
+           const Parameters::SearchValueType valueType);
+
+template <>
+bsls::Types::Uint64 getValue(const mqbs::JournalFileIterator*  jit,
+                             const Parameters::SearchValueType valueType)
+{
+    // PRECONDITIONS
+    BSLS_ASSERT(jit);
+    BSLS_ASSERT(valueType == Parameters::e_TIMESTAMP ||
+                valueType == Parameters::e_OFFSET);
+
+    return (valueType == Parameters::e_TIMESTAMP)
+               ? jit->recordHeader().timestamp()
+               : jit->recordOffset();
+}
+
+template <>
+CompositeSequenceNumber getValue(const mqbs::JournalFileIterator*  jit,
+                                 const Parameters::SearchValueType valueType)
+{
+    // PRECONDITIONS
+    BSLS_ASSERT(jit);
+    BSLS_ASSERT(valueType == Parameters::e_SEQUENCE_NUM);
+
+    return CompositeSequenceNumber(jit->recordHeader().primaryLeaseId(),
+                                   jit->recordHeader().sequenceNumber());
+}


I'd better make a comparator function and pass it to moveToLowerBound. It can significantly simplify the logic:

bool comparatorFunction(const mqbs::JournalFileIterator& jit, const Parameters& parameters) { if (valueType == Parameters::e_TIMESTAMP) { return jit.recordHeader().timestamp() > parameters.d_timestampGt; } else if () { ... } esle { ... } }

waldgange · 2024-11-15T13:07:01Z

src/applications/bmqstoragetool/m_bmqstoragetool_journalfileprocessor.cpp

+template <typename T>
+int moveToLowerBound(mqbs::JournalFileIterator*        jit,
+                     const Parameters::SearchValueType valueType,
+                     const T&                          valueGt)


int moveToLowerBound(mqbs::JournalFileIterator* jit, bsl::function<bool(const mqbs::JournalFileIterator&)>& comparator))

src/applications/bmqstoragetool/m_bmqstoragetool_journalfileprocessor.cpp

+            if (d_parameters->d_valueGt > 0) {
+                rc = moveToLowerBound<bsls::Types::Uint64>(
+                    iter,
+                    d_parameters->d_valueType,
+                    d_parameters->d_valueGt);
+            }
+            else {
+                rc = moveToLowerBound<CompositeSequenceNumber>(
+                    iter,
+                    d_parameters->d_valueType,
+                    d_parameters->d_seqNumGt);


waldgange · 2024-11-15T13:08:47Z

src/applications/bmqstoragetool/m_bmqstoragetool_parameters.h

+    bsls::Types::Uint64 d_valueGt;
+    // Filter messages greater than value
+    bsls::Types::Uint64 d_valueLt;
+    // Filter messages less than value
+    CompositeSequenceNumber d_seqNumGt;
+    // Filter messages greater than sequence number
+    CompositeSequenceNumber d_seqNumLt;
+    // Filter messages less than sequence number


This change looks confusing: There are 3 pairs of parameters, we store two of them in this struct and use only one pair. So let's keep all the three. We may need them in the future if our customers ask us to enable combined filtering. Moreover it will be really easy to implement.

waldgange · 2024-11-15T13:09:38Z

src/applications/bmqstoragetool/m_bmqstoragetool_compositesequencenumber.h

+
+inline bool CompositeSequenceNumber::isUnset() const
+{
+    return d_isUnset;


There are expressions like if (!isUnset()) {...} everywhere. Double negation looks overcomplicated to me. And furthermore nobody really "unsets" this object)). IsSet or IsInitialized would be much more readable.

Signed-off-by: Aleksandr Ivanov <[email protected]>

Signed-off-by: Alexander Ivanov <[email protected]>

Signed-off-by: Aleksandr Ivanov <[email protected]>

pniedzielski

Small changes, mostly typos. LGTM otherwise

pniedzielski · 2024-12-04T15:12:38Z

src/applications/bmqstoragetool/README.md

+                        [--seqnum-gt <composit sequence number greater than>]
+                        [--seqnum-lt <composit sequence number less than>]


Suggested change

[--seqnum-gt <composit sequence number greater than>]

[--seqnum-lt <composit sequence number less than>]

[--seqnum-gt <composite sequence number greater than>]

[--seqnum-lt <composite sequence number less than>]

pniedzielski · 2024-12-04T15:14:52Z

src/applications/bmqstoragetool/README.md

+       --seqnum-gt            <composit sequence number greater than>
+          lower composit sequence number bound, defined in form <leaseId-sequenceNumber>, e.g. 123-456
+       --seqnum-lt            <composit sequence number less than>
+          higher composit sequence number bound, defined in form <leaseId-sequenceNumber>, e.g. 123-456


Suggested change

--seqnum-gt <composit sequence number greater than>

lower composit sequence number bound, defined in form <leaseId-sequenceNumber>, e.g. 123-456

--seqnum-lt <composit sequence number less than>

higher composit sequence number bound, defined in form <leaseId-sequenceNumber>, e.g. 123-456

--seqnum-gt <composite sequence number greater than>

lower composite sequence number bound, defined in form <leaseId-sequenceNumber>, e.g. 123-456

--seqnum-lt <composite sequence number less than>

higher composite sequence number bound, defined in form <leaseId-sequenceNumber>, e.g. 123-456

pniedzielski · 2024-12-04T15:43:24Z

src/applications/bmqstoragetool/m_bmqstoragetool_compositesequencenumber.h

+    const m_bmqstoragetool::CompositeSequenceNumber& rhs)
+{
+    // PRECONDITIONS
+    BSLS_ASSERT(lhs.isSet() && rhs.isSet());


If CompositeSequenceNumber is value-semantic as documented, we probably want CompositeSequenceNumber() == CompositeSequenceNumber() to be true (for putting them in unordered containers, so that a = b implies a == b, etc). Similarly for operator< below. I suggest the ordering

CompositeSequenceNumber() < CompositeSequenceNumber(0, 0) < CompositeSequenceNumber(0, 1) < ... < CompositeSequenceNumber(1, 0) < CompositeSequenceNumber(1, 1) < ...

Given the above, this type becomes bsl::optional<bsl::pair<unsigned int, bsl::bsls::Types::Uint64> > with more interesting print and fromString functions. I think it makes sense to pull the bsl::optional part out and make this type not know about isSet.

Good catch, I was just inspired by MessageGUID class which has isUnset() and related logic. I propose to address your proposal in separate PR.

pniedzielski · 2024-12-04T16:08:45Z

src/applications/bmqstoragetool/README.md

+```bash
+bmqstoragetool --journal-file=<path> --seqnum=<leaseId-sequenceNumber_1> --seqnum=<leaseId-sequenceNumber_N>
+```
+NOTE: no other filters are allowed with this one


It seems reasonable to want to look at journal records with a particular primary lease ID that happened after a particular timestamp, no?

See earlier comment, let's make this change later.

At the first step we agreed to make these filters mutual exclusive, and then change behaviour depending on user feedback. Agree to make this change later.

pniedzielski · 2024-12-04T16:52:27Z

src/applications/bmqstoragetool/m_bmqstoragetool_filters.cpp

-        (d_timestampLt > 0 && ts >= d_timestampLt)) {
-        // Match by timestamp
+
+    // Apply `range` filter


If we're going to store the timestamp range, the offset range, and the composite sequence number range, what limits us to having only one type of range? E.g.,

bsls::Types::Uint64 timestamp = record.header().timestamp(); if ((d_range.d_timestampGt && timestamp <= *d_range.d_timestampGt) || (d_range.d_timestmapLt && timestamp >= *d_range.d_timestampLt)) { return false; } if ((d_range.d_offsetGt && offset <= *d_range.d_offsetGt) || (d_range.d_offsetLt && offset >= *d_range.d_offsetLt)) { return false; } CompositeSequenceNumber seqNum(record.header().primaryLeaseId(), record.header().sequenceNumber(); if ((d_range.d_seqNumGt && seqNum <= *d_range.d_seqNumGt) || (d_range.d_seqNumLt && seqNum >= *d_range.d_seqNumLt)) { return false; }

on the assumption that d_range members become bsl::optionals. This would seem to make the code both more simple and more flexible.

On second thought, let's save this for a later PR---it's a pretty simple change once this is in.

pniedzielski · 2024-12-04T16:56:12Z

src/applications/bmqstoragetool/m_bmqstoragetool_parameters.h

+        bsls::Types::Uint64 d_timestampGt;
+        // Filter messages greater than timestamp value
+        bsls::Types::Uint64 d_timestampLt;
+        // Filter messages less than timestamp value
+        bsls::Types::Uint64 d_offsetGt;
+        // Filter messages greater than offset value
+        bsls::Types::Uint64 d_offsetLt;
+        // Filter messages less than offset value
+        CompositeSequenceNumber d_seqNumGt;
+        // Filter messages greater than sequence number
+        CompositeSequenceNumber d_seqNumLt;
+        // Filter messages less than sequence number


All these should probably be bsl::optionals.

Will be addressed in separate PR later.

pniedzielski · 2024-12-18T22:03:29Z

src/applications/bmqstoragetool/m_bmqstoragetool_searchresult.h

+  private:
+    // PRIVATE DATA
+    bsl::vector<CompositeSequenceNumber> d_seqNums;
+    // List of composite sequence numbers to search for.


Most of the code uses this BDE-style format docs for data members, but it's because the conversion script did not catch them. I'm slowly converting this over, but it's a manual process (package by package in #523). For new code, let's use doxygen style for everything (/// before the members and Markdown).

Not a big deal, but less to change later.

pniedzielski · 2024-12-18T22:04:00Z

src/applications/bmqstoragetool/m_bmqstoragetool_searchresult.h

+    // ACCESSORS
+
+    bool stop(const CompositeSequenceNumber& sequenceNumber) const;
+    // Return 'true' if the specified 'sequenceNumber' is greater than


Doxygen format docs here.

pniedzielski · 2024-12-18T22:05:26Z

src/applications/bmqstoragetool/m_bmqstoragetool_searchresult.h

-    bool stop(bsls::Types::Uint64 timestamp) const;
-    // Return 'true' if the specified 'timestamp' is greated than
+    bool stop(const bsls::Types::Uint64 timestamp) const;
+    // Return 'true' if the specified 'timestamp' is greater than


I see you're keeping the existing doc format here, which makes sense, but let's take this opportunity to convert this into Doxygen format---probably should have been from the beginning.

Signed-off-by: Aleksandr Ivanov <[email protected]>

alexander-e1off · 2024-12-19T11:00:19Z

@pniedzielski thanks for your review! I've addressed typos and doxygen notes, other notes will be addressed in further PRs.

alexander-e1off changed the title ~~[Storagetool] Add searching by record sequence numbers and offsets~~ Feat [Storagetool]: add searching by record sequence numbers and offsets Nov 11, 2024

alexander-e1off force-pushed the storagetool-add-seqnum branch from 149c396 to 74bf139 Compare November 11, 2024 14:00

alexander-e1off changed the title ~~Feat [Storagetool]: add searching by record sequence numbers and offsets~~ Feat[Storagetool]: Add searching by record sequence numbers and offsets Nov 12, 2024

alexander-e1off force-pushed the storagetool-add-seqnum branch from 4f43a55 to 06ee86d Compare November 12, 2024 15:00

waldgange reviewed Nov 15, 2024

View reviewed changes

alexander-e1off force-pushed the storagetool-add-seqnum branch from 801619c to d40b2a6 Compare November 19, 2024 09:24

alexander-e1off marked this pull request as ready for review November 19, 2024 10:19

alexander-e1off requested a review from a team as a code owner November 19, 2024 10:19

alexander-e1off force-pushed the storagetool-add-seqnum branch 2 times, most recently from 7790015 to eae1cb5 Compare November 26, 2024 08:08

alexander-e1off added 15 commits November 27, 2024 10:43

Add seqnum and offset input args, add composite sequence number support

ca179b8

Signed-off-by: Aleksandr Ivanov <[email protected]>

Debug binary search for all range types

f36e2f6

Signed-off-by: Aleksandr Ivanov <[email protected]>

Reemove debug

2ed441f

Signed-off-by: Aleksandr Ivanov <[email protected]>

Simplify getValue template

23ca939

Signed-off-by: Aleksandr Ivanov <[email protected]>

Add search result decorators for seq.number and offset, add UTs

33d990e

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix hasCache() call

1bcf853

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix code style

25a805a

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix code style

9898b5d

Signed-off-by: Aleksandr Ivanov <[email protected]>

Add search for specific sequence numbers and offsets

d00d17c

Signed-off-by: Aleksandr Ivanov <[email protected]>

Cleanup

1435ed4

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix README.md

f1d28fa

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix typo in README.md

f69ee41

Signed-off-by: Aleksandr Ivanov <[email protected]>

Update header caption

6897db6

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix review comments

08e4983

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix code style

27194b8

Signed-off-by: Aleksandr Ivanov <[email protected]>

alexander-e1off force-pushed the storagetool-add-seqnum branch from eae1cb5 to 27194b8 Compare November 27, 2024 08:44

alexander-e1off added 6 commits November 29, 2024 16:26

Reset errorDescr

c8758ad

Signed-off-by: Aleksandr Ivanov <[email protected]>

Merge branch 'main' into storagetool-add-seqnum

7c234b9

Signed-off-by: Alexander Ivanov <[email protected]>

Fix merge conflicts

768b164

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix merge conflicts

6ecd167

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix formatting

10b24b5

Signed-off-by: Aleksandr Ivanov <[email protected]>

Merge remote-tracking branch 'upstream/main' into storagetool-add-seqnum

eab1197

alexander-e1off requested a review from kaikulimu December 4, 2024 09:13

pniedzielski self-assigned this Dec 4, 2024

alexander-e1off mentioned this pull request Dec 5, 2024

Feat[Storagetool]: Add support of queueOp and journalOp records #531

Draft

alexander-e1off added 5 commits December 5, 2024 11:15

Merge remote-tracking branch 'upstream/main' into storagetool-add-seqnum

3c7d12f

Merge remote-tracking branch 'upstream/main' into storagetool-add-seqnum

561f4da

Merge from main, fix conflicts

a290ba0

Signed-off-by: Aleksandr Ivanov <[email protected]>

Fix assert macro names in compositesequencenumber.t

e9cf8dd

Signed-off-by: Aleksandr Ivanov <[email protected]>

Merge remote-tracking branch 'upstream/main' into storagetool-add-seqnum

d17230a

pniedzielski requested changes Dec 18, 2024

View reviewed changes

Fix review comments: typos, doxygen comments

73cb8d0

Signed-off-by: Aleksandr Ivanov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat[Storagetool]: Add searching by record sequence numbers and offsets #508

Feat[Storagetool]: Add searching by record sequence numbers and offsets #508

alexander-e1off commented Nov 11, 2024 •

edited

Loading

waldgange Nov 15, 2024

This comment was marked as resolved.

waldgange Nov 15, 2024

This comment was marked as resolved.

This comment was marked as resolved.

waldgange Nov 15, 2024

This comment was marked as resolved.

waldgange Nov 15, 2024

This comment was marked as resolved.

pniedzielski left a comment

pniedzielski Dec 4, 2024

alexander-e1off Dec 19, 2024

pniedzielski Dec 4, 2024

alexander-e1off Dec 19, 2024

pniedzielski Dec 4, 2024

pniedzielski Dec 4, 2024

alexander-e1off Dec 19, 2024

pniedzielski Dec 4, 2024

pniedzielski Dec 18, 2024

alexander-e1off Dec 19, 2024

pniedzielski Dec 4, 2024

pniedzielski Dec 18, 2024

pniedzielski Dec 4, 2024

alexander-e1off Dec 19, 2024

pniedzielski Dec 18, 2024

pniedzielski Dec 18, 2024

pniedzielski Dec 18, 2024

alexander-e1off commented Dec 19, 2024

		[--seqnum-gt <composit sequence number greater than>]
		[--seqnum-lt <composit sequence number less than>]

Feat[Storagetool]: Add searching by record sequence numbers and offsets #508

Are you sure you want to change the base?

Feat[Storagetool]: Add searching by record sequence numbers and offsets #508

Conversation

alexander-e1off commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

pniedzielski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-e1off commented Dec 19, 2024

alexander-e1off commented Nov 11, 2024 •

edited

Loading