empiredan commented on code in PR #1608:
URL:
https://github.com/apache/incubator-pegasus/pull/1608#discussion_r1546271870
##########
src/replica/duplication/duplication_sync_timer.cpp:
##########
@@ -75,9 +79,13 @@ void duplication_sync_timer::run()
// collects confirm points from all primaries on this server
for (const replica_ptr &r : get_all_primaries()) {
Review Comment:
Actually both `get_all_primaries()` and `get_all_replicas()` are operate the
members of `replica_stub`, thus it's better to move both of them to
`replica_stub`. I'll create [another
pr](https://github.com/apache/incubator-pegasus/pull/1967) to refactor.
##########
src/replica/replica_stub.cpp:
##########
@@ -2805,5 +2805,15 @@ void replica_stub::wait_closing_replicas_finished()
}
}
+bool replica_stub::replica_is_cloing_or_closed(gpid id)
Review Comment:
Actually in `duplication_sync_timer::get_all_primaries()` and
`duplication_sync_timer::get_all_replicas()`, replicas in
`replica_stub::_replicas` would not be in `_closing_replicas` and
`_closed_replicas`. Thus `replica_is_cloing_or_closed` could be removed.
##########
src/replica/replica.h:
##########
@@ -239,7 +243,10 @@ class replica : public serverlet<replica>, public
ref_counter, public replica_ba
//
error_code trigger_manual_emergency_checkpoint(decree old_decree);
void on_query_last_checkpoint(learn_response &response);
- replica_duplicator_manager *get_duplication_manager() const { return
_duplication_mgr.get(); }
+ std::unique_ptr<replica_duplicator_manager> &get_duplication_manager()
Review Comment:
Actually, getting `_duplication_mgr` as a `std::unique_ptr` is still not
safe. For example `r->close()`would be called in `replica_stub::close_replica`
without any protection of lock. Once `_duplication_mgr.reset()` is called in
`replica::close()`(`r->close()`), `std::unique_ptr` would become `nullptr`,
which is not thread-safe among multiple threads. Since `_duplication_mgr` might
be called and reset in different threads, it's better to use `std::shared_ptr`
instead. Since this is not the problem which should be solved by this pr, I'll
create [another pr](https://github.com/apache/incubator-pegasus/pull/1968) to
fix this.
##########
src/replica/replica.h:
##########
@@ -162,6 +162,10 @@ class replica : public serverlet<replica>, public
ref_counter, public replica_ba
public:
~replica(void);
+ // store `info` into a file under `path` directory
+ // path = "" means using the default directory (`_dir`/.app_info)
+ error_code store_app_info(app_info &info, const std::string &path = "");
Review Comment:
`store_app_info` is not called by external objects, thus just keep private.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
