empiredan commented on code in PR #2373:
URL:
https://github.com/apache/incubator-pegasus/pull/2373#discussion_r2910096102
##########
src/replica/mutation_log.h:
##########
@@ -420,12 +425,18 @@ class mutation_log_private : public mutation_log, private
replica_base
_tracker.cancel_outstanding_tasks();
}
+ // Non-copyable and non-movable
+ mutation_log_private(const mutation_log_private &) = delete;
+ mutation_log_private &operator=(const mutation_log_private &) = delete;
+ mutation_log_private(mutation_log_private &&) = delete;
+ mutation_log_private &operator=(mutation_log_private &&) = delete;
Review Comment:
Please use `DISALLOW_COPY_AND_ASSIGN` and `DISALLOW_MOVE_AND_ASSIGN` declare
functions under `private:`.
##########
src/replica/mutation_log.h:
##########
@@ -420,12 +425,18 @@ class mutation_log_private : public mutation_log, private
replica_base
_tracker.cancel_outstanding_tasks();
}
+ // Non-copyable and non-movable
+ mutation_log_private(const mutation_log_private &) = delete;
+ mutation_log_private &operator=(const mutation_log_private &) = delete;
+ mutation_log_private(mutation_log_private &&) = delete;
+ mutation_log_private &operator=(mutation_log_private &&) = delete;
+
::dsn::task_ptr append(mutation_ptr &mu,
dsn::task_code callback_code,
dsn::task_tracker *tracker,
aio_handler &&callback,
- int hash = 0,
- int64_t *pending_size = nullptr) override;
+ int hash,
+ int64_t *pending_size) override;
Review Comment:
`append()` is inherited from class `mutation_log`. We'd better to keep both
functions in base and derived classes consistent.
##########
src/replica/mutation_log.h:
##########
@@ -400,9 +403,11 @@ class mutation_log : public ref_counter
// for plog. Since it is set with
mutation.data.header.last_committed_decree, it must
// be less than _plog_max_decree_on_disk.
decree _plog_max_commit_on_disk;
+
+ decree _cleanable_decree; // for gc crush
Review Comment:
Please add more detailed comments, such as explaining what the variable
represents, when it is used, and any important considerations, etc.
##########
src/replica/duplication/load_from_private_log.cpp:
##########
@@ -156,17 +157,38 @@ void load_from_private_log::find_log_file_to_start()
// Reopen the files. Because the internal file handle of `file_map`
// is cleared once WAL replay finished. They are unable to read.
mutation_log::log_file_map_by_index new_file_map;
- for (const auto &pr : file_map) {
+
+ decree cleanable_decree = _private_log->get_cleanable_decree();
+ decree max_decree_gpid = _private_log->max_decree(get_gpid());
+
+ if (max_decree_gpid <= cleanable_decree) {
+ LOG_ERROR_PREFIX("plog_file all error: max_decree_gpid {} ,
cleanable_decree {}",
Review Comment:
```suggestion
LOG_ERROR_PREFIX("max_decree_gpid({}) should be >
cleanable_decree({}) for plog",
```
##########
src/replica/duplication/load_from_private_log.cpp:
##########
@@ -156,17 +157,38 @@ void load_from_private_log::find_log_file_to_start()
// Reopen the files. Because the internal file handle of `file_map`
// is cleared once WAL replay finished. They are unable to read.
mutation_log::log_file_map_by_index new_file_map;
- for (const auto &pr : file_map) {
+
+ decree cleanable_decree = _private_log->get_cleanable_decree();
+ decree max_decree_gpid = _private_log->max_decree(get_gpid());
Review Comment:
```suggestion
const auto cleanable_decree = _private_log->get_cleanable_decree();
const auto max_decree_gpid = _private_log->max_decree(get_gpid());
```
##########
src/replica/duplication/load_from_private_log.cpp:
##########
@@ -156,17 +157,38 @@ void load_from_private_log::find_log_file_to_start()
// Reopen the files. Because the internal file handle of `file_map`
// is cleared once WAL replay finished. They are unable to read.
mutation_log::log_file_map_by_index new_file_map;
- for (const auto &pr : file_map) {
+
+ decree cleanable_decree = _private_log->get_cleanable_decree();
+ decree max_decree_gpid = _private_log->max_decree(get_gpid());
+
+ if (max_decree_gpid <= cleanable_decree) {
+ LOG_ERROR_PREFIX("plog_file all error: max_decree_gpid {} ,
cleanable_decree {}",
+ max_decree_gpid,
+ cleanable_decree);
+ return;
+ }
+
+ for (auto it = file_map.rbegin(); it != file_map.rend(); ++it) {
log_file_ptr file;
- error_s es = log_utils::open_read(pr.second->path(), file);
+ error_s es = log_utils::open_read(it->second->path(), file);
if (!es.is_ok()) {
LOG_ERROR_PREFIX("{}", es);
return;
}
- new_file_map.emplace(pr.first, file);
+
+ new_file_map.emplace(it->first, file);
+
+ // next file map may can not open
+ gpid pid = get_gpid();
+ decree previous_log_max_decree = file->previous_log_max_decree(pid);
+ // these plog_file has possible be deleted do not open_read next
plog_file , otherwise it
+ // may coredump
+ if (previous_log_max_decree <= cleanable_decree) {
+ break;
+ }
Review Comment:
```suggestion
// If the max decree of a log file falls within `cleanable_decree`,
the file may be deleted during the GC of plog files. Therefore, these files
should be skipped here.
if (cleanable_decree >= file->previous_log_max_decree(get_gpid())) {
break;
}
```
--
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]
