Skip to content
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

Lookup by appId #309

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Lookup by appId #309

merged 2 commits into from
Jun 3, 2024

Conversation

dorjesinpo
Copy link
Collaborator

@dorjesinpo dorjesinpo commented May 30, 2024

Avoid iteration in VirtualStorageCatalog::hasVirtualStorage(const bsl::string& appId by switching to TwoKeyHashMap
Preparatory step for Reliable Broadcast

@dorjesinpo dorjesinpo added the enhancement New feature or request label May 30, 2024
@dorjesinpo dorjesinpo requested a review from a team as a code owner May 30, 2024 22:17
@dorjesinpo dorjesinpo force-pushed the dev/VSC-TwoKeyHashMap branch from d8cdd77 to 33b870e Compare May 30, 2024 22:19
Signed-off-by: dorjesinpo <[email protected]>
@dorjesinpo dorjesinpo force-pushed the dev/VSC-TwoKeyHashMap branch from 33b870e to 11a7645 Compare May 30, 2024 22:33
@dorjesinpo dorjesinpo requested a review from 678098 May 30, 2024 23:22
678098
678098 previously approved these changes May 31, 2024
@@ -58,8 +61,9 @@ class VirtualStorageCatalog {
typedef bsl::shared_ptr<VirtualStorage> VirtualStorageSp;

/// appKey -> virtualStorage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to update the comment here.
/// Any(appId, appKey) -> virtualStorage

if (appKey) {
*appKey = it->first;
}
VirtualStoragesConstIter cit = d_virtualStorages.findByKey1(appId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good improvement

}

// Add guid to all virtual storages.

for (VirtualStoragesIter it = d_virtualStorages.begin();
it != d_virtualStorages.end();
++it) {
it->second->put(msgGUID, msgSize, rdaInfo, subScriptionId);
it->value()->put(msgGUID, msgSize, rdaInfo, subScriptionId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't check for return codes here, what if one or few put fails?
Currently, VirtualStorage::put might fail if GUID is not unique
On comparison, we might return e_GUID_NOT_UNIQUE on line 67

Copy link
Collaborator Author

@dorjesinpo dorjesinpo May 31, 2024

Choose a reason for hiding this comment

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

Looks like this codepath (appKey.isNull()) is for unique GUIDs only. The callers do not even check the return code.
We can add error code without affecting anything, I guess

@dorjesinpo dorjesinpo merged commit 51ad3c5 into main Jun 3, 2024
24 checks passed
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Lookup by appId

Signed-off-by: dorjesinpo <[email protected]>

* VirtualStorageCatalog::put returns error

Signed-off-by: dorjesinpo <[email protected]>

---------

Signed-off-by: dorjesinpo <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Lookup by appId

Signed-off-by: dorjesinpo <[email protected]>

* VirtualStorageCatalog::put returns error

Signed-off-by: dorjesinpo <[email protected]>

---------

Signed-off-by: dorjesinpo <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Lookup by appId

Signed-off-by: dorjesinpo <[email protected]>

* VirtualStorageCatalog::put returns error

Signed-off-by: dorjesinpo <[email protected]>

---------

Signed-off-by: dorjesinpo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants