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

[refactoring] remove find_sessoin with name #301

Merged

Conversation

SergeiNA
Copy link
Collaborator

@SergeiNA SergeiNA commented Jan 30, 2024

Removing string name field from session_key in dispatcher session.
Because of further refactoring require remove name usage in create/drop_index from dispatcher -> require session changes.

services/disk/manager_disk.cpp Show resolved Hide resolved
@@ -468,7 +468,7 @@ namespace services::dispatcher {
collection_full_name_t key(index.database_, index.collection_);
auto it_collection = collection_address_book_.find(key);
if (it_collection != collection_address_book_.end()) {
make_session(session_to_address_, session_key_t{session, index.name()}, session_t(address, index));
make_session(session_to_address_, session_key_t{session, {}}, session_t(address, index));
Copy link
Member

Choose a reason for hiding this comment

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

maybe change make_session(session_to_address_, session_key_t{session, {}}, session_t(address, index)); -> make_session(session_to_address_, session_key_t{session}, session_t(address, index)); ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm trying to do this. Looks like we have collision with multiple definition of make_session from collections.
I've removed string name from key, but left struct.

services/dispatcher/dispatcher.cpp Outdated Show resolved Hide resolved
*
* @return session_id_t new unique id
*/
static session_id_t generate_uid();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

explicit naming for new id generation

actor_zeta::send(dispatcher,
address(),
collection::handler_id(collection::route::create_index),
session,
session_id_t::generate_uid(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now it looks more self-documented

@@ -36,14 +36,13 @@ class session_t {
///components::session::session_id_t session_;
};

// TODO Remove this session and use logic from collections
// Move session logic from collections to a separate place
struct session_key_t {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Left structure with only one filed - not a great idea, but otherwise I have to do some general session refactoring.
@kotbegemot

@@ -257,10 +257,13 @@ namespace services::disk {
auto indexes = read_indexes_();
metafile_indexes_->seek_eof();
for (const auto& index : indexes) {
trace(log_, "manager_disk: load_indexes_ : {}", index.name());
// Require to separate sessions for load and create index
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kotbegemot left comment as you proceed.

@SergeiNA
Copy link
Collaborator Author

SergeiNA commented Feb 6, 2024

@kotbegemot Can I merge it?

- Session in dispather now use only session_id_t
- Small switch-case refactor
- In manager_disk we generate new session key
for each call of create index
@SergeiNA SergeiNA force-pushed the sn.remove-find_session-with-name branch from 89f29dc to 23e7107 Compare February 6, 2024 15:49
@SergeiNA SergeiNA merged commit 3fca254 into duckstax:develop Feb 6, 2024
3 of 12 checks passed
@SergeiNA SergeiNA deleted the sn.remove-find_session-with-name branch February 6, 2024 15:51
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.

None yet

2 participants