refactoring: Move insert_documents to execute ql #259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SergeiNA
commented
Oct 8, 2023
SergeiNA
commented
Oct 8, 2023
SergeiNA
force-pushed
the
sn-remove-insert_document
branch
from
eb43337
to
a82b13a
Compare
SergeiNA
force-pushed
the
sn-remove-insert_document
branch
3 times, most recently
from
43e5e37
to
1f1d995
Compare
* Now insert_one and insert_many are part of the execute_ql * Now we have to wait on wal response for all mutable operations * For update and delete we don't wait (will be fixed soon) * Now when wal is off we correctly process on empty manager_wal_replicate * Remove waiting signal from disk on remove collection
SergeiNA
force-pushed
the
sn-remove-insert_document
branch
from
1f1d995
to
4cc1e5b
Compare
SergeiNA
changed the title
SergeiNA
commented
Nov 29, 2023
| load_count_answers_ = records.size(); | ||
| void dispatcher_t::load_from_wal_result(components::session::session_id_t& session, std::vector<services::wal::record_t> in_records) { | ||
| // TODO think what to do with records | ||
| records_ = std::move(in_records); |
There was a problem hiding this comment.
@kotbegemot @makeyavelly plz pay attention to this. We decided to add this with @makeyavelly because before we got it as non const reference
void dispatcher_t::load_from_wal_result(components::session::session_id_t& session, std::vector<services::wal::record_t>& in_records)This potentially can lead to danling references.
SergeiNA
commented
Nov 29, 2023
| @@ -195,96 +198,83 @@ namespace services::dispatcher { | |||
| } | |||
|
|
|||
| void dispatcher_t::execute_ql_finish(components::session::session_id_t& session, const result_t& result) { | |||
| result_storage_[session] = result; | |||
There was a problem hiding this comment.
@kotbegemot @makeyavelly I've added this to be able to unlock wrapper_dsipathcer form wal_success
auto result = result_storage_[session];
actor_zeta::send(session_obj.address(), dispatcher_t::address(), handler_id(route::execute_ql_finish), session, result);I don't know how effective is it.
SergeiNA
commented
Nov 29, 2023
| void execute_ql(components::session::session_id_t& session, components::ql::ql_statement_t* ql, actor_zeta::address_t address); | ||
| void execute_ql_finish(components::session::session_id_t& session, const components::result::result_t& result); | ||
| void drop_collection_finish_from_disk(components::session::session_id_t& session, std::string& collection_name); |
There was a problem hiding this comment.
This no longer need -> we don't wait response form disk
3 tasks
SergeiNA
changed the title
kotbegemot
changed the title
makeyavelly
approved these changes
Dec 2, 2023
| auto session_obj = find_session(session_to_address_, session); | ||
| const bool is_from_wal = session_obj.address().get() == manager_wal_.get(); | ||
| if(is_from_wal){ | ||
| return; |
There was a problem hiding this comment.
Need to clean session`s storage:
remove_session(session_to_address_, session);
There was a problem hiding this comment.
I'm not sure - session will be removed here, isn't it?
if (!check_load_from_wal(session)) {
actor_zeta::send(s.address(), dispatcher_t::address(), handler_id(route::execute_ql_finish), session, result);
remove_session(session_to_address_, session);
}
There was a problem hiding this comment.
And looks like in this case one of the test is fail
There was a problem hiding this comment.
ottergon/integration/cpp/test/test_save_load.cpp:167: FAILED:
| @@ -64,7 +64,9 @@ namespace ottergon { | |||
| auto doc = to_document(document); | |||
| generate_document_id_if_not_exists(doc); | |||
| auto session_tmp = ottergon::session_id_t(); | |||
| auto result = ptr_->insert_one(session_tmp, database_, name_, doc); | |||
| auto result_variant = ptr_->insert_one(session_tmp, database_, name_, doc); | |||
| assert(result_variant.is_type<components::result::result_insert>() && "wrapper_collection::insert_one result error"); | |||
There was a problem hiding this comment.
Possible error_result_t
There was a problem hiding this comment.
How I should process it in this case? raise exception?
Added check
if(result_variant.is_error()){
debug(log_, "wrapper_collection::insert_one has result error while insert");
throw std::runtime_error("wrapper_collection::insert_one error_result");
}
@makeyavelly @makeyavelly PTAL
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main task Insert, delete and update should be part of execute_ql