You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "HolyLow (via GitHub)" <gi...@apache.org> on 2023/09/22 07:22:21 UTC

[GitHub] [kvrocks] HolyLow opened a new pull request, #1774: Add the support of the BLMPOP command.

HolyLow opened a new pull request, #1774:
URL: https://github.com/apache/kvrocks/pull/1774

   This closes [#1522](https://github.com/apache/kvrocks/issues/1522).
   
   This PR moves all the blocking-event related code to a upper class `BlockedPopCommand`, as all the blocking commands would share these utils. 
   If the `BlockedPopCommand` design gets accepted, I would try to refactor the other blocking commands, such as `BLPOP`, `BRPOP` and `BLMOVE` to reduce the code redundancy, just as commented by @git-hulk in [previous discussion](https://github.com/apache/kvrocks/issues/1522#issuecomment-1709660226).
   
   Any suggestions are welcomed.


-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kvrocks] PragmaTwice merged pull request #1774: Add the support of the BLMPOP command

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice merged PR #1774:
URL: https://github.com/apache/kvrocks/pull/1774


-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kvrocks] HolyLow commented on pull request #1774: Add the support of the BLMPOP command

Posted by "HolyLow (via GitHub)" <gi...@apache.org>.
HolyLow commented on PR #1774:
URL: https://github.com/apache/kvrocks/pull/1774#issuecomment-1731003278

   @PragmaTwice refactored to use `BlockingCommander`.


-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kvrocks] git-hulk commented on pull request #1774: Add the support of the BLMPOP command

Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1774:
URL: https://github.com/apache/kvrocks/pull/1774#issuecomment-1731152938

   > @PragmaTwice A CI test case failed with Ubuntu 20.04, clang, TLS on. The test case is in the tls_test (no command logic involved?), and I try my best to try to reproduce the problem on my local environment (Ubuntu22.04, clang-16, TLS on), but the error could not be reproduced...
   > 
   > I'm not aware what I should do now, could you please help?
   
   It's a flaky test case, can ignore this failure for now. We're still investigating.


-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kvrocks] PragmaTwice commented on pull request #1774: Add the support of the BLMPOP command.

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1774:
URL: https://github.com/apache/kvrocks/pull/1774#issuecomment-1730937041

   Hi, thank you for your contribution.
   
   Could you consider to rewrite it in our new helper class [BlockingCommander](https://github.com/apache/kvrocks/blob/unstable/src/commands/blocking_commander.h)?


-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kvrocks] HolyLow commented on pull request #1774: Add the support of the BLMPOP command

Posted by "HolyLow (via GitHub)" <gi...@apache.org>.
HolyLow commented on PR #1774:
URL: https://github.com/apache/kvrocks/pull/1774#issuecomment-1730942884

   @PragmaTwice seems we've done the same thing separately... Sure, I would refactor the code. As the design is very similar, it wouldn't take long.


-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kvrocks] HolyLow commented on pull request #1774: Add the support of the BLMPOP command

Posted by "HolyLow (via GitHub)" <gi...@apache.org>.
HolyLow commented on PR #1774:
URL: https://github.com/apache/kvrocks/pull/1774#issuecomment-1731148760

   @PragmaTwice A CI test case failed with Ubuntu 20.04, clang, TLS on. The test case is in the tls_test (no command logic involved?), and I try my best to try to reproduce the problem on my local environment (Ubuntu22.04, clang-16, TLS on), but the error could not be reproduced...
   
   I'm not aware what I should do now, could you please help?


-- 
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: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org