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 2020/09/23 17:06:24 UTC
[GitHub] [incubator-pegasus] neverchanje opened a new pull request #608: fix: bugfix on incr
neverchanje opened a new pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608
### What problem does this PR solve? <!--add issue link with summary if exists-->
https://github.com/apache/incubator-pegasus/pull/598 introduced a bug, which makes incr continue to proceed even the rocksdb::get failed.
### What is changed and how it works?
This PR fixes this bug and adds some unit tests using fail-point library to mock rocksdb get failure.
### Check List <!--REMOVE the items that are not applicable-->
Tests <!-- At least one of them must be included. -->
- Unit test
----------------------------------------------------------------
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.
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] Shuo-Jia commented on a change in pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608#discussion_r494010384
##########
File path: src/server/pegasus_write_service_impl.h
##########
@@ -182,59 +182,61 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
uint32_t new_expire_ts = 0;
db_get_context get_ctx;
int err = db_get(raw_key, &get_ctx);
- if (err == 0) {
- if (!get_ctx.found) {
- // old value is not found, set to 0 before increment
- new_value = update.increment;
- new_expire_ts = update.expire_ts_seconds > 0 ? update.expire_ts_seconds : 0;
- } else if (get_ctx.expired) {
- // ttl timeout, set to 0 before increment
- _pfc_recent_expire_count->increment();
+ if (err != 0) {
+ resp.error = err;
Review comment:
But you `return error` , `resp.error` is not be used.
##########
File path: src/server/pegasus_write_service_impl.h
##########
@@ -182,59 +182,61 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
uint32_t new_expire_ts = 0;
db_get_context get_ctx;
int err = db_get(raw_key, &get_ctx);
- if (err == 0) {
- if (!get_ctx.found) {
- // old value is not found, set to 0 before increment
- new_value = update.increment;
- new_expire_ts = update.expire_ts_seconds > 0 ? update.expire_ts_seconds : 0;
- } else if (get_ctx.expired) {
- // ttl timeout, set to 0 before increment
- _pfc_recent_expire_count->increment();
+ if (err != 0) {
+ resp.error = err;
Review comment:
Well, I konw
----------------------------------------------------------------
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.
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] neverchanje merged pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
neverchanje merged pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608
----------------------------------------------------------------
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.
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] Shuo-Jia commented on a change in pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608#discussion_r494010726
##########
File path: src/server/pegasus_write_service_impl.h
##########
@@ -182,59 +182,61 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
uint32_t new_expire_ts = 0;
db_get_context get_ctx;
int err = db_get(raw_key, &get_ctx);
- if (err == 0) {
- if (!get_ctx.found) {
- // old value is not found, set to 0 before increment
- new_value = update.increment;
- new_expire_ts = update.expire_ts_seconds > 0 ? update.expire_ts_seconds : 0;
- } else if (get_ctx.expired) {
- // ttl timeout, set to 0 before increment
- _pfc_recent_expire_count->increment();
+ if (err != 0) {
+ resp.error = err;
Review comment:
Well, I konw
----------------------------------------------------------------
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.
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] Shuo-Jia commented on a change in pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608#discussion_r494004203
##########
File path: src/server/pegasus_write_service_impl.h
##########
@@ -182,59 +182,61 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
uint32_t new_expire_ts = 0;
db_get_context get_ctx;
int err = db_get(raw_key, &get_ctx);
- if (err == 0) {
- if (!get_ctx.found) {
- // old value is not found, set to 0 before increment
- new_value = update.increment;
- new_expire_ts = update.expire_ts_seconds > 0 ? update.expire_ts_seconds : 0;
- } else if (get_ctx.expired) {
- // ttl timeout, set to 0 before increment
- _pfc_recent_expire_count->increment();
+ if (err != 0) {
+ resp.error = err;
Review comment:
The line seems to be useless?
----------------------------------------------------------------
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.
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] Shuo-Jia commented on a change in pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608#discussion_r494010384
##########
File path: src/server/pegasus_write_service_impl.h
##########
@@ -182,59 +182,61 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
uint32_t new_expire_ts = 0;
db_get_context get_ctx;
int err = db_get(raw_key, &get_ctx);
- if (err == 0) {
- if (!get_ctx.found) {
- // old value is not found, set to 0 before increment
- new_value = update.increment;
- new_expire_ts = update.expire_ts_seconds > 0 ? update.expire_ts_seconds : 0;
- } else if (get_ctx.expired) {
- // ttl timeout, set to 0 before increment
- _pfc_recent_expire_count->increment();
+ if (err != 0) {
+ resp.error = err;
Review comment:
But you `return error` , `resp.error` is not be used.
----------------------------------------------------------------
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.
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] Shuo-Jia commented on a change in pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
Shuo-Jia commented on a change in pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608#discussion_r494004203
##########
File path: src/server/pegasus_write_service_impl.h
##########
@@ -182,59 +182,61 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
uint32_t new_expire_ts = 0;
db_get_context get_ctx;
int err = db_get(raw_key, &get_ctx);
- if (err == 0) {
- if (!get_ctx.found) {
- // old value is not found, set to 0 before increment
- new_value = update.increment;
- new_expire_ts = update.expire_ts_seconds > 0 ? update.expire_ts_seconds : 0;
- } else if (get_ctx.expired) {
- // ttl timeout, set to 0 before increment
- _pfc_recent_expire_count->increment();
+ if (err != 0) {
+ resp.error = err;
Review comment:
The line seem to be useless?
----------------------------------------------------------------
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.
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] neverchanje commented on a change in pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
neverchanje commented on a change in pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608#discussion_r494006470
##########
File path: src/server/pegasus_write_service_impl.h
##########
@@ -182,59 +182,61 @@ class pegasus_write_service::impl : public dsn::replication::replica_base
uint32_t new_expire_ts = 0;
db_get_context get_ctx;
int err = db_get(raw_key, &get_ctx);
- if (err == 0) {
- if (!get_ctx.found) {
- // old value is not found, set to 0 before increment
- new_value = update.increment;
- new_expire_ts = update.expire_ts_seconds > 0 ? update.expire_ts_seconds : 0;
- } else if (get_ctx.expired) {
- // ttl timeout, set to 0 before increment
- _pfc_recent_expire_count->increment();
+ if (err != 0) {
+ resp.error = err;
Review comment:
Certainly useful. It sets response error.
----------------------------------------------------------------
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.
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] neverchanje merged pull request #608: fix: bugfix on incr
Posted by GitBox <gi...@apache.org>.
neverchanje merged pull request #608:
URL: https://github.com/apache/incubator-pegasus/pull/608
----------------------------------------------------------------
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.
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