You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/07/06 10:30:33 UTC
[GitHub] [incubator-pegasus] foreverneverer opened a new pull request, #1035: fix
foreverneverer opened a new pull request, #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035
### What problem does this PR solve? <!--add issue link with summary if exists-->
### What is changed and how does it work?
### Checklist <!--REMOVE the items that are not applicable-->
##### Tests <!-- At least one of them must be included. -->
- Unit test
- Integration test
- Manual test (add detailed scripts or steps below)
- No code
##### Code changes
- Has exported function/method change
- Has exported variable/fields change
- Has interface methods change
- Has persistent data change
##### Side effects
- Possible performance regression
- Increased code complexity
- Breaking backward compatibility
##### Related changes
- Need to cherry-pick to the release branch
- Need to update the documentation
- Need to be included in the release note
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916448515
##########
rdsn/src/common/replication_common.h:
##########
@@ -117,6 +117,11 @@ class replication_options
~replication_options();
void initialize();
+
+ int32_t min_2pc_replica_count(int32_t app_max_replica_count)
+ {
+ return std::min(mutation_2pc_min_replica_count, app_max_replica_count) - 1;
Review Comment:
Oh, the function should be removed...
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916453300
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
I retain _options->mutation_2pc_min_replica_count means that in the case: mutation_2pc_min_replica_count = 2, max_replica_count = 5, but the mim_2pc should equal with 2 but not 4, if you add `1` and then the mutation_2pc_min_replica_count is invalid.
But your case is right, so the function should:
```
assert(app_info.max_replica_count > 0);
return app_info.max_replica_count <=2 ? 1 : std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
```
Do you think it is ok？
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916680512
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Oh，great! Your suggestion is important.
So I modify like?
```c++
assert(app_info.max_replica_count > 0);
if (mutation_2pc_min_replica_count > 0) {// >0 means use the user config
return mutation_2pc_min_replica_count;
} else {// otherwise use the table max_replica_count
return app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count - 1
}
```
or remove the `mutation_2pc_min_replica_count`:
```c++
return app_info.max_replica_count <=2 ? 1 : 2
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r917583547
##########
rdsn/src/meta/meta_service.cpp:
##########
@@ -1306,5 +1306,15 @@ void meta_service::on_set_max_replica_count(configuration_set_max_replica_count_
server_state::sStateHash);
}
+int32_t meta_service::mutation_2pc_min_replica_count(int32_t app_max_replica_count) const
+{
+ dassert_f(_app_info.max_replica_count > 0);
+if (_opts.mutation_2pc_min_replica_count > 0) {// >0 means use the user config
+ return _opts.mutation_2pc_min_replica_count;
+} else {// otherwise based on the table max_replica_count
+ return app_max_replica_count <=2 ? 1 : app_max_replica_count / 2 + 1;
+}
Review Comment:
```suggestion
if (_opts.mutation_2pc_min_replica_count > 0) { // > 0 means use the user config
return _opts.mutation_2pc_min_replica_count;
} else { // otherwise based on the table max_replica_count
return app_max_replica_count <= 2 ? 1 : app_max_replica_count / 2 + 1;
}
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916506092
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Yeah, retain `_options->mutation_2pc_min_replica_count` is for user custom requirements, if user mark it `1` that means user hope just 1 replica is ok
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916507142
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
I suggest set `mutation_2pc_min_replica_count = 2` by default
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916680512
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Oh，great! Your suggestion is important.
So I modify like?
```c++
assert(app_info.max_replica_count > 0);
if (mutation_2pc_min_replica_count > 0) {// >0 means use the user config
return mutation_2pc_min_replica_count;
} else {// otherwise use the table max_replica_count
return app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count - 1
}
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916453300
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
I retain _options->mutation_2pc_min_replica_count means that in the case: mutation_2pc_min_replica_count = 2, max_replica_count = 5, but the mim_2pc should equal with 2 but not 4, if you add `1` , the mutation_2pc_min_replica_count is invalid and it always equal with 1
But your case is right, so the function should:
```
assert(app_info.max_replica_count > 0);
return app_info.max_replica_count <=2 ? 1 : std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
```
Do you think it is ok？
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r917589290
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,15 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count() const
+{
+ dassert_f(_app_info.max_replica_count > 0);
+if (_options->mutation_2pc_min_replica_count > 0) {// >0 means use the user config
+ return _options->mutation_2pc_min_replica_count;
+} else {// otherwise based on the table max_replica_count
+ return _app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count / 2 + 1;
+}
Review Comment:
```suggestion
if (_options->mutation_2pc_min_replica_count > 0) { // > 0 means use the user config
return _options->mutation_2pc_min_replica_count;
} else { // otherwise based on the table max_replica_count
return _app_info.max_replica_count <= 2 ? 1 : _app_info.max_replica_count / 2 + 1;
}
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916680512
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Oh，great! Your suggestion is important.
So I modify like?
```c++
assert(app_info.max_replica_count > 0);
if (mutation_2pc_min_replica_count > 0) {// >0 means use the user config
return mutation_2pc_min_replica_count;
} else {// otherwise use the table max_replica_count
return app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count - 1
}
```
or remove the `mutation_2pc_min_replica_count`, just:
```c++
return app_info.max_replica_count <=2 ? 1 : 2
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r918516377
##########
rdsn/src/meta/meta_service.cpp:
##########
@@ -1306,5 +1306,15 @@ void meta_service::on_set_max_replica_count(configuration_set_max_replica_count_
server_state::sStateHash);
}
+int32_t meta_service::mutation_2pc_min_replica_count(int32_t app_max_replica_count) const
+{
+ dassert_f(app_max_replica_count > 0, "max_replica_count > 0");
+ if (_opts.mutation_2pc_min_replica_count > 0) { // >0 means use the user config
+ return _opts.mutation_2pc_min_replica_count;
+ } else { // otherwise, the value based on the table max_replica_count
+ return app_max_replica_count <= 2 ? 1 : app_max_replica_count / 2 + 1;
+ }
+}
+
Review Comment:
I think we can encapsulate this logic into a function `int32_t mutation_2pc_min_replica_count(int32_t 2pc_min_replica_count, int32_t app_max_replica_count)`, and put in `rdsn/src/common/replication_common.h` ?
Therefore both functions can just be implemented like this:
```C++
int32_t meta_service::mutation_2pc_min_replica_count(int32_t app_max_replica_count) const
{
return mutation_2pc_min_replica_count(_opts.mutation_2pc_min_replica_count, app_max_replica_count);
}
int32_t replica::mutation_2pc_min_replica_count() const
{
return mutation_2pc_min_replica_count(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count);
}
```
##########
rdsn/src/meta/meta_service.cpp:
##########
@@ -1306,5 +1306,15 @@ void meta_service::on_set_max_replica_count(configuration_set_max_replica_count_
server_state::sStateHash);
}
+int32_t meta_service::mutation_2pc_min_replica_count(int32_t app_max_replica_count) const
+{
+ dassert_f(app_max_replica_count > 0, "max_replica_count > 0");
Review Comment:
dcheck_gt(app_max_replica_count, 0);
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r917500004
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
I think both methods can work well with most cases. Maybe it's better to reserve `mutation_2pc_min_replica_count` for special cases, so we can adopt first method ? And while `mutation_2pc_min_replica_count` is not enabled, use N/2+1 instead (no special reason, since PacificA doesn't need quorum write) ?
```
assert(_app_info.max_replica_count > 0);
if (mutation_2pc_min_replica_count > 0) {// >0 means use the user config
return mutation_2pc_min_replica_count;
} else {// otherwise use the table max_replica_count
return app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count / 2 + 1;
}
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r917600445
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,15 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count() const
+{
+ dassert_f(_app_info.max_replica_count > 0);
+if (_options->mutation_2pc_min_replica_count > 0) {// >0 means use the user config
+ return _options->mutation_2pc_min_replica_count;
+} else {// otherwise based on the table max_replica_count
+ return _app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count / 2 + 1;
+}
Review Comment:
format done
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916516770
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
OK, I understand. I think in most cases setting `mutation_2pc_min_replica_count` to 2 is ok.
Is there a scenario that user sets `mutation_2pc_min_replica_count` to 3 while `max_replica_count` is 3 ? In this case the real `mutation_2pc_min_replica_count` will return 2. Should we handle this scenario ?
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r917583547
##########
rdsn/src/meta/meta_service.cpp:
##########
@@ -1306,5 +1306,15 @@ void meta_service::on_set_max_replica_count(configuration_set_max_replica_count_
server_state::sStateHash);
}
+int32_t meta_service::mutation_2pc_min_replica_count(int32_t app_max_replica_count) const
+{
+ dassert_f(_app_info.max_replica_count > 0);
+if (_opts.mutation_2pc_min_replica_count > 0) {// >0 means use the user config
+ return _opts.mutation_2pc_min_replica_count;
+} else {// otherwise based on the table max_replica_count
+ return app_max_replica_count <=2 ? 1 : app_max_replica_count / 2 + 1;
+}
Review Comment:
```suggestion
if (_opts.mutation_2pc_min_replica_count > 0) { // > 0 means use the user config
return _opts.mutation_2pc_min_replica_count;
} else { // otherwise based on the table max_replica_count
return app_max_replica_count <= 2 ? 1 : app_max_replica_count / 2 + 1;
}
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
acelyc111 merged PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916680512
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Oh，great! Your suggestion is important.
So I modify like:
```c++
assert(app_info.max_replica_count > 0);
if (mutation_2pc_min_replica_count > 0) {// >0 means use the user config
return mutation_2pc_min_replica_count;
} else {// otherwise use the table max_replica_count
return app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count - 1
}
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#issuecomment-1181280810
Should we also add new `mutation_2pc_min_replica_count()` into `partition_health_status()` ? Since it also uses `mutation_2pc_min_replica_count` to decide if a partition is unwritable:
```c++
inline health_status partition_health_status(const partition_configuration &pc,
int mutation_2pc_min_replica_count)
{
if (pc.primary.is_invalid()) {
if (pc.secondaries.empty())
return HS_DEAD;
else
return HS_UNREADABLE;
} else { // !pc.primary.is_invalid()
int n = pc.secondaries.size() + 1;
if (n < mutation_2pc_min_replica_count)
return HS_UNWRITABLE;
else if (n < pc.max_replica_count)
return HS_WRITABLE_ILL;
else
return HS_HEALTHY;
}
}
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r917507444
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
OK, I agree!
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916435517
##########
rdsn/src/replica/replica.h:
##########
@@ -497,6 +497,8 @@ class replica : public serverlet<replica>, public ref_counter, public replica_ba
void update_app_max_replica_count(int32_t max_replica_count);
+ int32_t mutation_2pc_min_replica_count();
Review Comment:
```suggestion
int32_t mutation_2pc_min_replica_count() const;
```
##########
rdsn/src/common/replication_common.h:
##########
@@ -117,6 +117,11 @@ class replication_options
~replication_options();
void initialize();
+
+ int32_t min_2pc_replica_count(int32_t app_max_replica_count)
+ {
+ return std::min(mutation_2pc_min_replica_count, app_max_replica_count) - 1;
Review Comment:
Should be `return std::min(mutation_2pc_min_replica_count, app_max_replica_count - 1);` ?
##########
rdsn/src/common/replication_common.h:
##########
@@ -117,6 +117,11 @@ class replication_options
~replication_options();
void initialize();
+
+ int32_t min_2pc_replica_count(int32_t app_max_replica_count)
+ {
+ return std::min(mutation_2pc_min_replica_count, app_max_replica_count) - 1;
Review Comment:
Should be `return std::min(mutation_2pc_min_replica_count, app_max_replica_count - 1);` ?
##########
rdsn/src/common/replication_common.h:
##########
@@ -117,6 +117,11 @@ class replication_options
~replication_options();
void initialize();
+
+ int32_t min_2pc_replica_count(int32_t app_max_replica_count)
Review Comment:
```suggestion
int32_t min_2pc_replica_count(int32_t app_max_replica_count) const
```
##########
rdsn/src/meta/meta_service.h:
##########
@@ -174,6 +174,8 @@ class meta_service : public serverlet<meta_service>
return metas.substr(0, metas.length() - 1);
}
+ int32_t mutation_2pc_min_replica_count(int32_t app_max_replica_count);
Review Comment:
```suggestion
int32_t mutation_2pc_min_replica_count(int32_t app_max_replica_count) const;
```
##########
rdsn/src/common/replication_common.h:
##########
@@ -117,6 +117,11 @@ class replication_options
~replication_options();
void initialize();
+
+ int32_t min_2pc_replica_count(int32_t app_max_replica_count)
+ {
+ return std::min(mutation_2pc_min_replica_count, app_max_replica_count) - 1;
Review Comment:
Should be `return std::min(mutation_2pc_min_replica_count, app_max_replica_count - 1);` ?
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916449578
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Should this be compared as follows ?
```c++
return std::min({_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1, 1});
```
Since for single replica `max_replica_count` is 1, then `std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1)` will be 0.
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r918635954
##########
rdsn/src/common/replication_common.cpp:
##########
@@ -230,7 +230,8 @@ void replication_options::initialize()
"replication",
"mutation_2pc_min_replica_count",
mutation_2pc_min_replica_count,
- "minimum number of alive replicas under which write is allowed");
+ "minimum number of alive replicas under which write is allowed. it's valid if larger than "
+ "0, otherwise, the final value based app max replica count");
Review Comment:
```suggestion
"0, otherwise, the final value is based on app_max_replica_count");
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916439512
##########
rdsn/src/common/replication_common.h:
##########
@@ -117,6 +117,11 @@ class replication_options
~replication_options();
void initialize();
+
+ int32_t min_2pc_replica_count(int32_t app_max_replica_count)
+ {
+ return std::min(mutation_2pc_min_replica_count, app_max_replica_count) - 1;
Review Comment:
yeah, the latest may be not pushed...
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916453300
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
I retain _options->mutation_2pc_min_replica_count means that in the case: mutation_2pc_min_replica_count = 2, max_replica_count = 5, but the mim_2pc should equal with 2 but not 4, if you add `1` , the mutation_2pc_min_replica_count is invalid and it always equal with 1
But your case is right, so the function should:
```c++
assert(app_info.max_replica_count > 0);
return app_info.max_replica_count <=2 ? 1 : std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
```
Do you think it is ok？
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
empiredan commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916503997
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Yeah, this made sense. However, another case is that the replica count is increased from 1 to 3: since `_options->mutation_2pc_min_replica_count` is originally set to 1, `std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1)` will still return 1. Does this means we have to modify `_options->mutation_2pc_min_replica_count` ?
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] foreverneverer commented on a diff in pull request #1035: fix(update_replication_factor#12): update the mutation_2pc_min_replica_count base the max count of table
Posted by GitBox <gi...@apache.org>.
foreverneverer commented on code in PR #1035:
URL: https://github.com/apache/incubator-pegasus/pull/1035#discussion_r916680512
##########
rdsn/src/replica/replica.cpp:
##########
@@ -581,5 +581,10 @@ error_code replica::store_app_info(app_info &info, const std::string &path)
return err;
}
+int32_t replica::mutation_2pc_min_replica_count()
+{
+ return std::min(_options->mutation_2pc_min_replica_count, _app_info.max_replica_count - 1);
Review Comment:
Oh，great! Your suggestion is important.
So I modify like?
```c++
assert(app_info.max_replica_count > 0);
if (mutation_2pc_min_replica_count > 0) {// >0 means use the user config
return mutation_2pc_min_replica_count;
} else {// otherwise use the table max_replica_count
return app_info.max_replica_count <=2 ? 1 : _app_info.max_replica_count - 1
}
```
or remove the `mutation_2pc_min_replica_count`, just:
```c++
assert(app_info.max_replica_count > 0);
return app_info.max_replica_count <=2 ? 1 : 2
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org