redis command splitter refactor by danielhochman · Pull Request #1273 · envoyproxy/envoy

danielhochman · 2017-07-17T23:28:10Z

This is a refactor to move logic from the handler into the request. This will make it possible to create an abstract base class for fragmented commands (MGET + MSET, DEL, EXISTS, TOUCH, etc) to share some of the splitting logic.

dnoe

Minor nit about unique_ptr initialization.

dnoe · 2017-07-18T14:01:33Z

source/common/redis/command_splitter_impl.cc

+                                      const RespValue& incoming_request,
+                                      SplitCallbacks& callbacks) {
+  std::unique_ptr<SimpleRequest> request_ptr =
+      std::unique_ptr<SimpleRequest>{new SimpleRequest(callbacks)};


Why not std::unique_ptr<SimpleRequest> request_ptr{new SimpleRequest(callbacks)}; ?

dnoe · 2017-07-18T14:06:58Z

source/common/redis/command_splitter_impl.cc

+SplitRequestPtr MGETRequest::create(ConnPool::Instance& conn_pool,
+                                    const RespValue& incoming_request, SplitCallbacks& callbacks) {
+  std::unique_ptr<MGETRequest> request_ptr =
+      std::unique_ptr<MGETRequest>{new MGETRequest(callbacks)};


Here too, can you just do:

std::unique_ptr<MGETRequest> request_ptr{new MGETRequest(callbacks)}

mattklein123

Looks good, some small comments.

mattklein123 · 2017-07-18T15:04:10Z

source/common/redis/command_splitter_impl.cc

+  return std::move(request_ptr);
 }

-AllParamsToOneServerCommandHandler::SplitRequestImpl::~SplitRequestImpl() { ASSERT(!handle_); }


This ASSERT should remain. I would add back the destructor and the assert.

mattklein123 · 2017-07-18T15:05:29Z

source/common/redis/command_splitter_impl.cc

-  pending_requests_.reserve(num_responses);
-}
-
-MGETCommandHandler::SplitRequestImpl::~SplitRequestImpl() {


Same. Retain destructor and all ASSERTS (if there are others that I am missing).

fixed both cases

mattklein123 · 2017-07-18T15:06:06Z

source/common/redis/command_splitter_impl.cc

-    ENVOY_LOG(debug, "redis: response: '{}'", pending_response_->toString());
+  ASSERT(num_pending_responses_ > 0);
+  if (--num_pending_responses_ == 0) {
+    // ENVOY_LOG(debug, "redis: response: '{}'", pending_response_->toString());


Why is this commented out?

mattklein123 · 2017-07-18T15:11:26Z

source/common/redis/command_splitter_impl.h

-  struct SplitRequestImpl : public SplitRequest, public ConnPool::PoolCallbacks {
-    SplitRequestImpl(SplitCallbacks& callbacks) : callbacks_(callbacks) {}
-    ~SplitRequestImpl();
+  void onFailure();


All overrides should have "override" postfix, del newline between definitions, and add // Redis::ConnPool::PoolCallbacks between them. Same in other places.

mattklein123 · 2017-07-18T15:12:21Z

source/common/redis/command_splitter_impl.h

-class MGETCommandHandler : public CommandHandler,
-                           CommandHandlerBase,
-                           Logger::Loggable<Logger::Id::redis> {
+class MGETRequest : public SplitRequest {


Add class comment

mattklein123 · 2017-07-18T15:12:30Z

source/common/redis/command_splitter_impl.h

+  uint32_t num_pending_responses_;
+};
+
+template <class RequestClass>


Add class comment

mattklein123 · 2017-07-18T17:20:31Z

source/common/redis/command_splitter_impl.h

+  static SplitRequestPtr create(ConnPool::Instance& conn_pool, const RespValue& incoming_request,
+                                SplitCallbacks& callbacks);
+
+  void onChildResponse(RespValuePtr&& value, uint32_t index);


I think these 2 methods can be private?

mattklein123 · 2017-07-18T17:21:02Z

source/common/redis/command_splitter_impl.cc

+    PendingRequest& pending_request = request_ptr->pending_requests_.back();

-    single_mget.asArray()[1].asString() = request.asArray()[i].asString();
-    ENVOY_LOG(debug, "redis: parallel get: '{}'", single_mget.toString());


Why remove this debug print?

dnoe · 2017-07-18T17:28:45Z

source/common/redis/command_splitter_impl.cc


+MGETRequest::~MGETRequest() {
+#ifndef NDEBUG
+  for (const PendingRequest& request : pending_requests_) {


I'd be curious if the compiler optimizer is smart enough to remove the entire loop when ASSERT is compiled out rendering this #ifdef unnecessary.

I kind of doubt it, but I would be curious also. :)

Prepare to be impressed. I compiled this with Clang++ -O2 -S to see the assembly (using cassert, but it should be equivalent to our macro):

void do_nothing_to_vector(std::vector<std::string>& foo) { for (const std::string& s : foo) { assert(s.size() > 1); } }

If you add -DNDEBUG to the Clang command line the assembly it generates is just "retq"

mattklein123

LGTM. I don't care either way about the NDEBUG removal. (I guess my preference would be to not depend on a compiler optimization in this case, but I don't care that much). @dnoe can decide whether he cares or not.

dnoe

I think leaving the ifdef here is fine. It's a slightly ugly leak of the assert implementation, but it makes it obvious to someone reading the code that the loop is gone in opt builds.

Description: Standardizes source and test paths across languages. Risk Level: Moderate Testing: Local and CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This is a follow up on #1248, and enables the skipped test case by using the old commit hash. We will pin it later to the next version which is v0.4.0. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Daniel Hochman added 5 commits July 13, 2017 12:27

refactor logic from handler to request

1ff19aa

fix format

f612f7f

move to ::create

07125f0

back to cc

3591f93

fix tests

a99f621

danielhochman requested a review from mattklein123 July 17, 2017 23:28

dnoe reviewed Jul 18, 2017

View reviewed changes

mattklein123 reviewed Jul 18, 2017

View reviewed changes

address comments

36f5b80

mattklein123 reviewed Jul 18, 2017

View reviewed changes

dnoe reviewed Jul 18, 2017

View reviewed changes

make child methods private, re-add debug log

e2c7d76

mattklein123 approved these changes Jul 18, 2017

View reviewed changes

dnoe approved these changes Jul 18, 2017

View reviewed changes

danielhochman merged commit d56c6d2 into master Jul 18, 2017

danielhochman deleted the redis-splitter-refactor branch July 18, 2017 18:32

Conversation

danielhochman commented Jul 17, 2017

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants