Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use const ConnectionContext in VerifyCommand #1633

Merged
merged 5 commits into from
Aug 6, 2023
Merged

feat: Use const ConnectionContext in VerifyCommand #1633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6e73d21
feat: Use const ConnectionContext in VerifyCommand
dranikpg Aug 3, 2023
c6be83d
fix: move variant unwrapping
dranikpg Aug 3, 2023
f80f1b9
fix: fixes
dranikpg Aug 3, 2023
c94998d
fix: fixes v2
dranikpg Aug 3, 2023
f79e619
feat: Add SendError overload for capturing rb
dranikpg Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/facade/facade_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@
#include <absl/container/flat_hash_map.h>
#include <absl/types/span.h>

#include <optional>
#include <string>
#include <string_view>
#include <variant>

#include "facade/op_status.h"

namespace facade {

Expand Down Expand Up @@ -56,6 +60,24 @@ struct ConnectionStats {
ConnectionStats& operator+=(const ConnectionStats& o);
};

struct ErrorReply {
explicit ErrorReply(std::string&& msg, std::string_view kind = {})
: message{move(msg)}, kind{kind} {
}
explicit ErrorReply(std::string_view msg, std::string_view kind = {}) : message{msg}, kind{kind} {
}
explicit ErrorReply(const char* msg,
std::string_view kind = {}) // to resolve ambiguity of constructors above
: message{std::string_view{msg}}, kind{kind} {
}
explicit ErrorReply(OpStatus status) : message{}, kind{}, status{status} {
}

std::variant<std::string, std::string_view> message;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can have message of type string_view, and some other member (like private message_memory_) which message may or may not point into?
It should make using this struct easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember you don't like using variants for storage at all, we already had this case once.
But I like to avoid self referential structs - it requires defining move/copy constructors and operators

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not worried about performance implications of using std::string in case we return errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I use string_view whenever its possible, in other cases you construct a string with StrCat and others anyways

I'll check once again for performance degradation, but it shouldn't be the case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw I once thought of supporting an absl::Span<absl::Alphanumeric> interface to write directly into the conn buffer without the intermediary string, but its just a random comment in this context 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yes, I don't see any place where the number of intermediaries increased

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant - I am NOT worried - i.e. I think having a string here is perfectly fine since it's not on a hot path.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any problem with storing variant as a member, or using it as storage. I actually think variants are great.
I dislike the API that it brings in, though. I definitely understand your comment re/ having to implement an explicit move / copy operations.
I guess we could:

  1. Keep as is
  2. Provide a wrapper get-method that returns string_view which points to the right string
  3. do my trick, but have the string inside a unique_ptr
  4. always use a string (Roman's suggestion)
    Your call :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you decided to keep as is. I would still say it's not a great API to force a call to std::visit with a templated lambda instead of having a

string_view GetMessage() const { visit... }

To wrap that, but again, your call :)

For the case where you want to move if possible, you could have another version (GetMessageDestructive()?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😝
As soon as it'll be used more than in once place I'll add those

std::string_view kind;
std::optional<OpStatus> status{std::nullopt};
};

inline MutableSlice ToMSS(absl::Span<uint8_t> span) {
return MutableSlice{reinterpret_cast<char*>(span.data()), span.size()};
}
Expand Down
8 changes: 8 additions & 0 deletions src/facade/reply_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,14 @@ void RedisReplyBuilder::SendError(string_view str, string_view err_type) {
}
}

void RedisReplyBuilder::SendError(ErrorReply error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that you move anything from error, so can you accept it by const&?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. Forgot to comment about it. I want the CapturingReplyBuilder to move out of the string so it can re-use its memory. The downside is that it requires passing the error by value everyewhere to comply with the interface

if (error.status)
return SendError(*error.status);

string_view message_sv = visit([](auto&& str) -> string_view { return str; }, error.message);
SendError(message_sv, error.kind);
}

void RedisReplyBuilder::SendProtocolError(std::string_view str) {
SendError(absl::StrCat("-ERR Protocol error: ", str), "protocol_error");
}
Expand Down
3 changes: 3 additions & 0 deletions src/facade/reply_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <optional>
#include <string_view>

#include "facade/facade_types.h"
#include "facade/op_status.h"
#include "io/io.h"

Expand Down Expand Up @@ -174,6 +175,8 @@ class RedisReplyBuilder : public SinkReplyBuilder {
void SetResp3(bool is_resp3);

void SendError(std::string_view str, std::string_view type = {}) override;
virtual void SendError(ErrorReply error);

void SendMGetResponse(absl::Span<const OptResp>) override;

void SendStored() override;
Expand Down
7 changes: 7 additions & 0 deletions src/facade/reply_capture.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ void CapturingReplyBuilder::SendError(std::string_view str, std::string_view typ
Capture(Error{str, type});
}

void CapturingReplyBuilder::SendError(ErrorReply error) {
SKIP_LESS(ReplyMode::ONLY_ERR);

string message = visit([](auto&& str) -> string { return string{move(str)}; }, error.message);
Capture(Error{move(message), error.kind});
}

void CapturingReplyBuilder::SendMGetResponse(absl::Span<const OptResp> arr) {
SKIP_LESS(ReplyMode::FULL);
Capture(vector<OptResp>{arr.begin(), arr.end()});
Expand Down
1 change: 1 addition & 0 deletions src/facade/reply_capture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class CapturingReplyBuilder : public RedisReplyBuilder {

public:
void SendError(std::string_view str, std::string_view type = {}) override;
void SendError(ErrorReply error) override;
void SendMGetResponse(absl::Span<const OptResp>) override;

// SendStored -> SendSimpleString("OK")
Expand Down
15 changes: 15 additions & 0 deletions src/server/command_registry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,21 @@ void CommandId::Invoke(CmdArgList args, ConnectionContext* cntx) const {
ent.second += (after - before) / 1000;
}

optional<facade::ErrorReply> CommandId::Validate(CmdArgList args) const {
if ((arity() > 0 && args.size() != size_t(arity())) ||
(arity() < 0 && args.size() < size_t(-arity()))) {
return facade::ErrorReply{facade::WrongNumArgsError(name()), kSyntaxErrType};
}

if (key_arg_step() == 2 && (args.size() % 2) == 0) {
return facade::ErrorReply{facade::WrongNumArgsError(name()), kSyntaxErrType};
}

if (validator_)
return validator_(args.subspan(1));
return nullopt;
}

CommandRegistry::CommandRegistry() {
vector<string> rename_command = GetFlag(FLAGS_rename_command);

Expand Down
26 changes: 12 additions & 14 deletions src/server/command_registry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <absl/types/span.h>

#include <functional>
#include <optional>

#include "base/function2.hpp"
#include "facade/command_id.h"
Expand Down Expand Up @@ -66,11 +67,16 @@ class CommandId : public facade::CommandId {
void(CmdArgList, ConnectionContext*) const>;

using ArgValidator = fu2::function_base<true, true, fu2::capacity_default, false, false,
bool(CmdArgList, ConnectionContext*) const>;
std::optional<facade::ErrorReply>(CmdArgList) const>;

bool is_multi_key() const {
return (last_key_ != first_key_) || (opt_mask_ & CO::VARIADIC_KEYS);
}
void Invoke(CmdArgList args, ConnectionContext* cntx) const;

// Returns error if validation failed, otherwise nullopt
std::optional<facade::ErrorReply> Validate(CmdArgList args) const;

bool IsTransactional() const;

static const char* OptName(CO::CommandOpt fl);

CommandId& SetHandler(Handler f) {
handler_ = std::move(f);
Expand All @@ -79,21 +85,13 @@ class CommandId : public facade::CommandId {

CommandId& SetValidator(ArgValidator f) {
validator_ = std::move(f);

return *this;
}

void Invoke(CmdArgList args, ConnectionContext* cntx) const;

// Returns true if validation succeeded.
bool Validate(CmdArgList args, ConnectionContext* cntx) const {
return !validator_ || validator_(std::move(args), cntx);
bool is_multi_key() const {
return (last_key_ != first_key_) || (opt_mask_ & CO::VARIADIC_KEYS);
}

bool IsTransactional() const;

static const char* OptName(CO::CommandOpt fl);

private:
Handler handler_;
ArgValidator validator_;
Expand Down
Loading