Skip to content

Refactor[mqbs::FileStore]: Encapsulate write-head cursor accessors - #1673

Merged
kaikulimu merged 6 commits into
bloomberg:mainfrom
kaikulimu:write-head-refactor
Jul 31, 2026
Merged

Refactor[mqbs::FileStore]: Encapsulate write-head cursor accessors#1673
kaikulimu merged 6 commits into
bloomberg:mainfrom
kaikulimu:write-head-refactor

Conversation

@kaikulimu

@kaikulimu kaikulimu commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Signed-off-by: Yuan Jing Vincent Yan <yyan82@bloomberg.net>
Signed-off-by: Yuan Jing Vincent Yan <yyan82@bloomberg.net>
Signed-off-by: Yuan Jing Vincent Yan <yyan82@bloomberg.net>
@kaikulimu
kaikulimu requested a review from a team as a code owner July 30, 2026 20:00
@kaikulimu
kaikulimu requested a review from 678098 July 30, 2026 20:02
{
return d_highestSeqNums[d_writeHeadLeaseId];
d_writeHeadLeaseId = leaseId;
d_highestSeqNums[leaseId] = seqNum;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this method has any preconditions such as d_writeHeadLeaseId <= leaseId?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not yet, in legacy mode in FileStore::clearPrimary() there is a write head rollbacks mechanism. I am trying to eliminate rollbacks but will do it across a series of PRs to make the changes gradual.

Comment thread src/groups/mqb/mqbs/mqbs_filestore.h Outdated
inline bsls::Types::Uint64& FileStore::currentSeqNumRef()
inline bsls::Types::Uint64 FileStore::incrementWriteHeadSeqNum()
{
return ++d_highestSeqNums.at(d_writeHeadLeaseId);

@678098 678098 Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

.at might throw an exception and the current design choice is not to throw exceptions in the code:

    at(const LOOKUP_KEY& key) {
        HashTableLink *node = d_impl.find(key);

        if (!node) {
            BloombergLP::bslstl::StdExceptUtil::throwOutOfRange(
                      "unordered_map<...>::at(LOOKUP_KEY): invalid key value");
        }
        return static_cast<HashTableNode *>(node)->value().second;
    }

What about changing it to

auto iter = d_highestSeqNums.find(d_writeHeadLeaseId);
BSLS_ASSRT_SAFE(iter != d_highestSeqNums.end());
++iter.second;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No auto in C++03, but I will do the assert instead.

Comment thread src/groups/mqb/mqbs/mqbs_filestore.cpp Outdated
qlistRecTotalLength);

DataStoreRecordKey key(sequenceNumber(), d_writeHeadLeaseId);
DataStoreRecordKey key(writeHeadSeqNum(), d_writeHeadLeaseId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Found it weird that DataStoreRecordKey has inverted order of args compared to basically everything in this file, for example:

            setWriteHead(recHeader->primaryLeaseId(),
                         recHeader->sequenceNumber());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I can also fix it in this PR. Still pure refactoring so won't be too much.

@678098 678098 assigned kaikulimu and unassigned 678098 Jul 30, 2026
Signed-off-by: Yuan Jing Vincent Yan <yyan82@bloomberg.net>
Signed-off-by: Yuan Jing Vincent Yan <yyan82@bloomberg.net>
@kaikulimu kaikulimu assigned 678098 and unassigned kaikulimu Jul 31, 2026

unsigned int d_primaryLeaseId;

bsls::Types::Uint64 d_sequenceNum;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Now member alignment is not ideal:

d_primaryLeaseId - 4 bytes
[empty space] - 4 bytes
d_sequenceNum - 8 bytes

If only we could use 64bit type for leaseId too...

@kaikulimu kaikulimu Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Switched them back only in the data member declarations.

Comment thread src/groups/mqb/mqbs/mqbs_datastore.h Outdated
DataStoreRecordKey(const bsls::Types::Uint64 sequenceNum,
unsigned int primaryLeaseId);
DataStoreRecordKey(unsigned int primaryLeaseId,
const bsls::Types::Uint64 sequenceNum);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const bsls::Types::Uint64 sequenceNum);
bsls::Types::Uint64 sequenceNum);

This is a basic type, no need to have const

Comment thread src/groups/mqb/mqbs/mqbs_filestore.h Outdated
Comment on lines +1167 to +1170
BSLS_ASSERT_SAFE(d_highestSeqNums.find(d_writeHeadLeaseId) !=
d_highestSeqNums.end());

return ++d_highestSeqNums[d_writeHeadLeaseId];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There are 2 lookups now when we can have 1 for better performance.

BSLS_ASSERT_SAFE(d_highestSeqNums.find(d_writeHeadLeaseId) !=
                     d_highestSeqNums.end());

This check searches for iterator and immediately drops it, when it can be reused to increment the value

@678098 678098 assigned kaikulimu and unassigned 678098 Jul 31, 2026
Signed-off-by: Yuan Jing Vincent Yan <yyan82@bloomberg.net>
@kaikulimu kaikulimu assigned 678098 and unassigned kaikulimu Jul 31, 2026

@678098 678098 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good

@678098 678098 assigned kaikulimu and unassigned 678098 Jul 31, 2026
@kaikulimu
kaikulimu merged commit 82ee395 into bloomberg:main Jul 31, 2026
48 checks passed
@kaikulimu
kaikulimu deleted the write-head-refactor branch July 31, 2026 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants