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.

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

Redis API in Scylla #5132

Merged
merged 4 commits into from Nov 20, 2019
Merged

Redis API in Scylla #5132

merged 4 commits into from Nov 20, 2019

Conversation

@fastio
Copy link
Contributor

fastio commented Oct 2, 2019

The detailed design and implementation of Redis API in Scylla is provided in the doc/redis/redis.md.
The redis commands (such as PING, GET, SET, DEL) are already implemented.

redis_server_config _config;
size_t _max_request_size;
semaphore _memory_available;
seastar::metrics::metric_groups _metrics;

This comment has been minimized.

Copy link
@amnonh

amnonh Oct 2, 2019

Contributor

I see that you define a metric_gruup object, which is good, but I don't see any metrics defined.

Adding metrics can be in a follow-up patch, what is important is that you'll have a counter (i.e. ever-growing unsigned int) for each of your commands, so it can be monitored separately.

Also, I suggest monitoring the latencies of your read and write, take and store a timestamp at the beginning of each operation and use an estimated_histogram for latencies calculations.

This comment has been minimized.

Copy link
@fastio

fastio Oct 4, 2019

Author Contributor

Hi @amnonh , thanks for your suggestion about metrics.
Now, only some basic metrics are added.

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

By the way, you can see examples of metrics similar to what you may want to add in Alternator - alternator/stats.{cc,hh}

@asias

This comment has been minimized.

Copy link
Contributor

asias commented Oct 9, 2019

  • The commit message has extra space. Run git log and compare the commit message with other commits in the tree you will notice.

  • Commit 7e549dc and 1ef0f1b can be merged together.

  • Commit 4d67fae is the only commit that adds a individual command. To make the review easier, you can organize the commits as 1) first introduce the infrastructure 2) then implement the individual redis commands 3) enable the redis option and wire it up.

commit 4d67faeee85ca71e21d6fa7441d3201c890c1d84 (fastio/master)
Author: Peng Jian <pengjian.uestc@gmail.com>
Date:   Fri Oct 4 19:45:48 2019 +0800

        ADD select command.
    
        process the exception when received wrong arguments.
    
        Signed-off-by: Peng Jian, <pengjian.uestc@gmail.com>

commit 7e549dc1bff866949f820e7cc0899f2a7c3686fb
Author: Peng Jian <pengjian.uestc@gmail.com>
Date:   Fri Oct 4 15:11:17 2019 +0800

        Redis API in Scylla
    
        Remove unnecessary logs.
        Add metrics for for Redis API.
    
        Signed-off-by: Peng Jian, <pengjian.uestc@gmail.com>

commit 1ef0f1b15c7bb65d8849902543352153a4be5634
Author: Peng Jian <pengjian.uestc@gmail.com>
Date:   Wed Oct 2 17:29:42 2019 +0800

        Redis API in Scylla
    
        Scylla has advantage and amazing features. If Redis build on the top of Scylla,
        it has the above features automatically. It's achived great progress
        in cluster master managment, data persistence, failover and replication.
    
        The benefits to the users are easy to use and develop in their production
        environment, and taking avantages of Scylla.
    
        Using the Ragel to parse the Redis request, server abtains the command name
        and the parameters from the request, invokes the Scylla's internal API to
        read and write the data, then replies to client.
    
        Signed-off-by: Peng Jian, <pengjian.uestc@gmail.com>
seastar::metrics::metric_groups _metrics;
private:
uint64_t _connects = 0;
uint64_t _connections = 0;

This comment has been minimized.

Copy link
@amoskong

amoskong Oct 12, 2019

Contributor

Is it repeated with _total_connections & _current_connections in line 145?

    uint64_t _total_connections = 0;
    uint64_t _current_connections = 0;

This comment has been minimized.

Copy link
@amoskong

amoskong Oct 12, 2019

Contributor

BTW, it's difficult to know the difference between those two variables from the name.

This comment has been minimized.

Copy link
@fastio

fastio Oct 14, 2019

Author Contributor

total_connections, means that this is an incremental counter, while current_connections means that current active client connections counter which increased when connection is connected, reduced when the connection is closed.

main.cc Outdated
@@ -1089,6 +1092,17 @@ int main(int ac, char** av) {
startlog.info("Alternator server listening on {} port {}", addr, cfg->alternator_port());
}

if (cfg->enable_redis_protocol()) {
supervisor::notify("starting create redis databases if not exists.");
redis::create_keyspace_if_not_exists(cfg).get();

This comment has been minimized.

Copy link
@amoskong

amoskong Oct 12, 2019

Contributor

alternator has a similar method executor::maybe_create_keyspace() in alternator/executor.cc

And that's included in start() stage.

This comment has been minimized.

Copy link
@fastio

fastio Oct 14, 2019

Author Contributor

Thanks. I will read the code of executor::maybe_create_keyspace. Maybe it 's useful for me.

This comment has been minimized.

Copy link
@fastio

fastio Oct 14, 2019

Author Contributor

Thanks for your suggestion.

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

Note that the Alternator logic is to delay the keyspace creation until the first time we want to create a table - hoping that by that time we know if the cluster is big enough to use RF=3. Your logic is different (the user needs to configure RF manually, and the first node to start controls it) so I don't know if you'll want to, or can, reuse Alternator code for this..

This comment has been minimized.

Copy link
@fastio

fastio Oct 29, 2019

Author Contributor

It's different from the case of Alternator. It's better to create the keyspaces/tables in the start stage before the server serving the requests.

Copy link
Contributor

nyh left a comment

Hi, thanks for this patch series. I think it's a much better shape than the previous iteration, but I left quite a few comments and requests for your consideration that I think can improve the code and its future maintainability. Please also take a look at the other reviewers' comments and requests.

persistence. Now Redis as memory store service is provided by many cloud
platforms.

**IMPORTANT NOTE**: As we known, Redis owns many useful features. Storing the

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

I didn't understand this important note. Is the note that you only implement "storing basic data structures" and not something else (what?)? It's not clear to me what you're trying to say here.

This comment has been minimized.

Copy link
@fastio

fastio Oct 29, 2019

Author Contributor

Sorry, this IMPORTANT NOT is not necessary here. And it should be removed.
What I want to say is that suporting the Redis API for storing data is enough to many users.
Of course, we can support all of the Redis API.

docs/redis/redis.md Show resolved Hide resolved
command is received, it's processed and a reply is sent back to the client.
Two kind of Redis clients are widely used, the smart Redis client and
simple Redis client. When using smart Redis client to connect the Redis
cluster, it will send the commaonds to the right Redis server. Because the

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

commaonds -> commands

This comment has been minimized.

Copy link
@fastio

fastio Oct 29, 2019

Author Contributor

Got it.

client can accesses the Redis cluster built on the top of Scylla, when
provided address is not an IP but rather a single domain name, which a
client resloves a random Scylla node IP address. The smart Redis client
will query the mapping information between hash slots and Server nodes. We

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

"will query the mapping" - please clarify if this is this a feature you already provide in this patch, or by using future tense you intended to say that this feature isn't implemented yet. If it's not implemented yet, please say this explicitly.

This comment has been minimized.

Copy link
@fastio

fastio Oct 30, 2019

Author Contributor

This is a syntax error. Actually, I have implemented the command named 'CLUSTER SLOTS' in the peids project. The redis client uses this command to query the mapping of hash slots and server nodes.
I'll fix this.

will query the mapping information between hash slots and Server nodes. We
just fill the mapping with random Scylla node IP address.

With Redis enabled on port 6379 (which is configurable), every Scylla node

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

would be nice to say the configuration name instead of just "is configurable", and say whether the default is that it's off (in other words, it's not just "configurable", it actually must be configured to be run).

This comment has been minimized.

Copy link
@fastio

fastio Oct 30, 2019

Author Contributor

OK. I agree it.

redis_server_config _config;
size_t _max_request_size;
semaphore _memory_available;
seastar::metrics::metric_groups _metrics;

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

By the way, you can see examples of metrics similar to what you may want to add in Alternator - alternator/stats.{cc,hh}

@@ -3554,5 +3555,108 @@ db::schema_features storage_service::cluster_schema_features() const {
return f;
}

} // namespace service
future<> storage_service::start_redis_transport(distributed<redis::query_processor>& qp) {

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

Can this code please moved to one of the source files in redis/? Maybe redis/server.cc or something?
I'd really like to see most of the redis-related code in redis/, most of the DynamoDB-specific code in alternator/, etc. storage_service.cc is already too huge.

This comment has been minimized.

Copy link
@fastio

fastio Oct 30, 2019

Author Contributor

Yes, of course.

@@ -50,6 +50,23 @@ redis_server::redis_server(distributed<service::storage_proxy>& proxy, distribut
, _memory_available(_max_request_size)
, _auth_service(auth_service)
{

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

Please use a different title for each patch. If this patch is about statistics, title it something like "Redis API: add statistics".

This comment has been minimized.

Copy link
@fastio

fastio Oct 30, 2019

Author Contributor

OK. I got it.

@@ -50,6 +50,23 @@ redis_server::redis_server(distributed<service::storage_proxy>& proxy, distribut
, _memory_available(_max_request_size)
, _auth_service(auth_service)
{
namespace sm = seastar::metrics;

_metrics.add_group("redis_transport", {

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

If this patch is about adding statistics, maybe you can move log-related stuff from the previous huge patch, to this one.
Not critical, though.

This comment has been minimized.

Copy link
@fastio

fastio Oct 30, 2019

Author Contributor

OK.

@@ -163,15 +180,12 @@ future<> redis_server::connection::process()
f.get();
}
catch (redis_exception& e) {
logging.info(" redis exception = {} ", e.what());

This comment has been minimized.

Copy link
@nyh

nyh Oct 28, 2019

Contributor

You can drop these lines from the original patch - you don't need to add them in patch 2 and drop them in patch 3.

This comment has been minimized.

Copy link
@fastio

fastio Oct 30, 2019

Author Contributor

Got it.

@avikivity

This comment has been minimized.

Copy link
Contributor

avikivity commented Oct 28, 2019

I have a question about pipelining. Suppose we have SET x aaa followed by GET x. Will the two commands run in parallel or sequentially? My understanding that in redis they will run sequentially.

Perhaps we should serialize accesses to the same key, but allow accesses to different keys to run in parallel.

@nyh

This comment has been minimized.

Copy link
Contributor

nyh commented Oct 30, 2019

@fastio I have another request.

It's fairly obvious that while you were developing this code, you needed to test its functionality - test that the protocol is actually parsed correctly, that responses and errors are generated in a way that Redis clients understand them, that writing data and reading it back actually works - and so on. You must have done such tests yourself, either manually or programatically in some way. I would really like to see automated test that check these things.

These test will be super-important in the future when you, or somebody else, modifies the Redis-support code or even general Scylla code and want to verify that we didn't break anything in Redis support.
Though you can argue that we can add such tests later, I believe that we need to add at least a seed for them - even if just a small amount of test cases - now - for this seed to grow later. If we don't start adding such tests now, it is unlikely we'll ever have them.

In the current Scylla codebase, you can find two example of how we did such functional tests and you can use one of them - or propose another one if you had something else in mind. The older approach you can see in tests/cql_query_test.cc. This is C++ code linked with Scylla (so test executables are pretty huge, unfortunately), a test quickly starts one-shard Scylla server - and then does all sorts of tests on it using various undocumented C++-code hooks into Scylla. The second, newer, approach is used in testing Alternator (the DynamoDB API) which you can find in the alternator-test/ directory: This approach has the user starting Scylla manually, and then test code written in any language (in alternator-test we used Python) use a standard client library to connect to Scylla via the API being tested (in alternator-test, we used Amazon's "boto3" library which connects to DynamoDB).

In my opinion, the second approach has two important benefits over the first which is why I chose it for Alternator. First it makes writing tests very easy - almost fun - which encourages better test coverage. Tests are easy to write Python code, don't need lengthy compilation, and the pytest framework we used for Python tests make it very easy to manage the tests, understand where they failed, etc. Second, we can run the same tests against both Scylla and the server we're trying to emulate (DynamoDB or Redis). This is important, if you want to make sure that Scylla's behavior not only does not regress, it was actually right in the first place.

We actually have a third testing approach in Scylla, called "dtest", which also has tests in Python, but these tests start their own Scylla clusters. However, these tests are so incredibly slow (since each small test starts a separate Scylla cluster) that I would recommend against this third approach.

@fastio

This comment has been minimized.

Copy link
Contributor Author

fastio commented Oct 30, 2019

@avikivity Yes, you are right. All the commands in redis received by one redis-server are exectued sequentially.

Perhaps we should serialize accesses to the same key, but allow accesses to different keys to run in parallel.

I agree with you. We can submit the requests with the same key to the same shard by the hash of key.

@fastio

This comment has been minimized.

Copy link
Contributor Author

fastio commented Oct 30, 2019

@nyh Thank you for your advice. The codes should be tested. And it's very important for production system. Actually, there are several test cases in the pedis project . I agree with you to use the second approach to write test cases for Redis API.

main.cc Outdated
@@ -1121,12 +1113,6 @@ int main(int ac, char** av) {
api::set_server_done(ctx).get();
supervisor::notify("serving");
// Register at_exit last, so that storage_service::drain_on_shutdown will be called first
auto stop_redis_query_processor = defer([&redis_query_processor, cfg] {

This comment has been minimized.

Copy link
@nyh

nyh Nov 13, 2019

Contributor

How is this destruction code not needed any more?

This comment has been minimized.

Copy link
@fastio

fastio Nov 14, 2019

Author Contributor

The shutdown hook for redis is added now.

@fastio

This comment has been minimized.

Copy link
Contributor Author

fastio commented Nov 13, 2019

Change logs:

  • Add redis/service.{hh, cc}: move the Redis transport related codes from service/storage_service.{hh,cc} to redis/service.{hh, cc}.
  • Add redis/stat.{hh,cc}: implement the basic metrics for Redis API.
  • Add the test cases: the test codes written in Python using the redis-py library.
  • Fix the reply of the DEL command.
  • Fix some errors and incorrect description in the doc/redis/redis.md.

@nyh . Thanks for review.

@fastio

This comment has been minimized.

Copy link
Contributor Author

fastio commented Nov 14, 2019

Change logs:

  • Add the shutdown hook for redis to stop redis service.
auto pkey = partition_key::from_single_value(*schema, key);
auto m = mutation(schema, std::move(pkey));
m.partition().apply(tombstone { api::new_timestamp(), gc_clock::now() });
return std::move(m);

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 15, 2019

Contributor
FAILED: build/release/redis/mutation_utils.o 
g++ -MD -MT build/release/redis/mutation_utils.o -MF build/release/redis/mutation_utils.o.d -I/home/amos/scylladb.com/scylla-build-example/scylla-redis/seastar/include -I/home/amos/scylladb.com/scylla-build-example/scylla-redis/build/release/seastar/gen/include -std=gnu++17 -U_FORTIFY_SOURCE -Wno-maybe-uninitialized -march=westmere -Werror=unused-result -fconcepts -DSEASTAR_API_LEVEL=2 -DSEASTAR_HAVE_GCC6_CONCEPTS -DSEASTAR_USE_STD_OPTIONAL_VARIANT_STRINGVIEW -DSEASTAR_HAVE_DPDK -DFMT_SHARED -I/usr/include/p11-kit-1 -ffile-prefix-map=/home/amos/scylladb.com/scylla-build-example/scylla-redis=.  -march=westmere -DBOOST_TEST_DYN_LINK -DTHRIFT_USES_BOOST  -fvisibility=hidden -Wall -Werror -Wno-maybe-uninitialized -Wno-tautological-compare -Wno-missing-braces -Wno-misleading-indentation -Wno-overflow -Wno-noexcept-type -Wno-nonnull-compare -Wno-error=cpp -Wno-ignored-attributes -Wno-overloaded-virtual -Wno-stringop-overflow -Wno-error=deprecated-declarations -DXXH_PRIVATE_API -DSEASTAR_TESTING_MAIN -DHAVE_LIBSYSTEMD=1 -DHAVE_LZ4_COMPRESS_DEFAULT -O3 --param inline-unit-growth=300 -gz  -g -I. -I build/release/gen  -c -o build/release/redis/mutation_utils.o redis/mutation_utils.cc
redis/mutation_utils.cc: In function 'mutation redis::make_mutation(service::storage_proxy&, const redis::redis_options&, bytes&&, bytes&&, long int)':
redis/mutation_utils.cc:61:21: error: moving a local object in a return statement prevents copy elision [-Werror=pessimizing-move]
   61 |     return std::move(m);
      |            ~~~~~~~~~^~~
redis/mutation_utils.cc:61:21: note: remove 'std::move' call
redis/mutation_utils.cc: In function 'mutation redis::make_tombstone(service::storage_proxy&, const redis::redis_options&, const sstring&, const bytes&)':
redis/mutation_utils.cc:77:21: error: moving a local object in a return statement prevents copy elision [-Werror=pessimizing-move]
   77 |     return std::move(m);
      |            ~~~~~~~~~^~~
redis/mutation_utils.cc:77:21: note: remove 'std::move' call
cc1plus: all warnings being treated as errors

This comment has been minimized.

Copy link
@fastio

fastio Nov 16, 2019

Author Contributor

Thanks. The std::move is unnecessary. I'll fix it.

}%%

class redis_protocol_parser : public ragel_parser_base<redis_protocol_parser> {
%% write data nofinal noprefix;

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 15, 2019

Contributor
FAILED: build/release/redis/service.o 
g++ -MD -MT build/release/redis/service.o -MF build/release/redis/service.o.d -I/home/amos/scylladb.com/scylla-build-example/scylla-redis/seastar/include -I/home/amos/scylladb.com/scylla-build-example/scylla-redis/build/release/seastar/gen/include -std=gnu++17 -U_FORTIFY_SOURCE -Wno-maybe-uninitialized -march=westmere -Werror=unused-result -fconcepts -DSEASTAR_API_LEVEL=2 -DSEASTAR_HAVE_GCC6_CONCEPTS -DSEASTAR_USE_STD_OPTIONAL_VARIANT_STRINGVIEW -DSEASTAR_HAVE_DPDK -DFMT_SHARED -I/usr/include/p11-kit-1 -ffile-prefix-map=/home/amos/scylladb.com/scylla-build-example/scylla-redis=.  -march=westmere -DBOOST_TEST_DYN_LINK -DTHRIFT_USES_BOOST  -fvisibility=hidden -Wall -Werror -Wno-maybe-uninitialized -Wno-tautological-compare -Wno-missing-braces -Wno-misleading-indentation -Wno-overflow -Wno-noexcept-type -Wno-nonnull-compare -Wno-error=cpp -Wno-ignored-attributes -Wno-overloaded-virtual -Wno-stringop-overflow -Wno-error=deprecated-declarations -DXXH_PRIVATE_API -DSEASTAR_TESTING_MAIN -DHAVE_LIBSYSTEMD=1 -DHAVE_LZ4_COMPRESS_DEFAULT -O3 --param inline-unit-growth=300 -gz  -g -I. -I build/release/gen  -c -o build/release/redis/service.o redis/service.cc
In file included from ./redis/server.hh:39,
                 from redis/service.cc:24:
build/release/gen/redis/protocol_parser.hh:43:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_targs []' of incomplete type
   43 |  static const char _nfa_targs[] = {
      |                    ^~~~~~~~~~
build/release/gen/redis/protocol_parser.hh:47:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_offsets []' of incomplete type
   47 |  static const char _nfa_offsets[] = {
      |                    ^~~~~~~~~~~~
build/release/gen/redis/protocol_parser.hh:53:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_push_actions []' of incomplete type
   53 |  static const char _nfa_push_actions[] = {
      |                    ^~~~~~~~~~~~~~~~~
build/release/gen/redis/protocol_parser.hh:57:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_pop_trans []' of incomplete type
   57 |  static const char _nfa_pop_trans[] = {
      |                    ^~~~~~~~~~~~~~

This comment has been minimized.

Copy link
@fastio

fastio Nov 16, 2019

Author Contributor

Hi @amoskong , what's the version of ragel which you used?

I use the ragel with version 6.8, and there are no errors as you mentioned.

Ragel State Machine Compiler version 6.8 Feb 2013
Copyright (c) 2001-2009 by Adrian Thurston

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 16, 2019

Contributor

I built by ./tools/toolchain/dbuild ./reloc/build_reloc.sh.

[scylla-redis]$ ./tools/toolchain/dbuild -it -- rpm -q ragel
ragel-7.0.0.11-3.fc30.x86_64

This comment has been minimized.

Copy link
@fastio

fastio Nov 16, 2019

Author Contributor

I found that the same problem was reported in this issue: scylladb/seastar#296

This comment has been minimized.

Copy link
@fastio

fastio Nov 16, 2019

Author Contributor

I change the ragel to latest version (Ragel State Machine Compiler version 7.0.0.12 May 2019). The errors are still exists. But the ragel with version 6.8 works fine.

Can we change the ragel version to 6.8 ?

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

I change the ragel to latest version (Ragel State Machine Compiler version 7.0.0.12 May 2019). The errors are still exists. But the ragel with version 6.8 works fine.

Can we change the ragel version to 6.8 ?

/CC @avikivity ?

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

The building completed without error after downgraded ragel to 6.8-5

The downgrade command:

sudo dnf downgrade -y https://rpmfind.net/linux/mageia/distrib/5/x86_64/media/core/release/ragel-6.8-5.mga5.x86_64.rpm

New docker image with ragel-6.8-5

diff --git a/tools/toolchain/image b/tools/toolchain/image
index 3d89f8f..2e9992e 100644
--- a/tools/toolchain/image
+++ b/tools/toolchain/image
@@ -1 +1 @@
-docker.io/scylladb/scylla-toolchain:fedora-30-20190911
+docker.io/amoskong/scylla-toolchain:fedora-30-ragel6.8

This comment has been minimized.

Copy link
@nyh

nyh Nov 17, 2019

Contributor

I don't follow the details of this discussion (I'm not at all familiar with Ragel or what changed in its versions), but please don't make your code rely on an antique version of Ragel. My build machine has ragel 7.0.0.11 and I don't have a problem building Seastar of Scylla. The new code needs to work also on the modern Ragel 7.0 - it can't just work on Ragel 6.8. That's not a viable workaround, in my opinion.

@fastio

This comment has been minimized.

Copy link
Contributor Author

fastio commented Nov 16, 2019

Change logs:

  • Fix the warnings being treated as errors: remove unnecessary std::move.
static future<redis_message> exception(const redis_exception& e) {
return from_exception(make_message("-ERR %s\r\n", e.what_message()));
}
static future<redis_message> make_exception(const sstring& em) {

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

It's not making exception, but make a redis message from exception message.

How about this? then we don't need from_exception(), we can consider exception() is one kind of redis_message.

    static future<redis_message> exception(const sstring& em) {
        auto m = make_lw_shared<scattered_message<char>> ();
        m->append(make_message("-ERR %s\r\n", em));
        return make_ready_future<redis_message>(m);
    }
    static future<redis_message> exception(const redis_exception& e) {
        return exception(e.what_message());
    }

This comment has been minimized.

Copy link
@fastio

fastio Nov 17, 2019

Author Contributor

I agree with you. I'll change it.

return make_ready_future<redis_message>(m);
}
static future<redis_message> zero() {
return err();

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

Calling zero() in err() is better. BTW err message is a general message, there are many kind of error, not just return zero.

    static future<redis_message> zero() {
        auto m = make_lw_shared<scattered_message<char>> ();
        m->append_static(":0\r\n");
        return make_ready_future<redis_message>(m);
    }
    static future<redis_message> err() {
        return zero();
    }

This comment has been minimized.

Copy link
@fastio

fastio Nov 17, 2019

Author Contributor

Yes. It's better to call zeor() in err().

if (result->has_result()) {
return redis_message::make_strings_result(std::move(result->result()));
}
return redis_message::err();

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

Currently the err() will return zero ":0\r\n".

Quote from https://redis.io/commands/get

Return value
Bulk string reply: the value of key, or nil when key does not exist.

The correct response message should be "$-1\r\n"

Referenced https://redis.io/topics/protocol:

RESP Bulk Strings can also be used in order to signal non-existence of a value using a special format that is used to represent a Null value. In this special format the length is -1, and there is no data, so a Null is represented as:

"$-1\r\n"
This is called a Null Bulk String.

The client library API should not return an empty string, but a nil object, when the server replies with a Null Bulk String. For example a Ruby library should return 'nil' while a C library should return NULL (or set a special flag in the reply object), and so forth.

This comment has been minimized.

Copy link
@fastio

fastio Nov 17, 2019

Author Contributor

Yes, you are right. Here we should returns nil string if key does not exist.

future<redis_message> del::execute(service::storage_proxy& proxy, redis::redis_options& options, service_permit permit)
{
return redis::delete_object(proxy, options, std::move(_key), permit).then([] {
return redis_message::one();

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

Currently your implement only supports to delete one key once.
Redis supports to delete multiple keys once: https://redis.io/commands/del

It returns the number of deleted keys, not always to be one: ":1\r\n"

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

zero ":0\r\n" should be returned if the key doesn't exist.

This comment has been minimized.

Copy link
@fastio

fastio Nov 17, 2019

Author Contributor

It's easy to delete multi keys.
In fact, we should determine that how many keys should be actually deleted. This is a read-before-write operation. As we known that Scylla does not support LWT. So should we just read all keys, then delete the exist keys?

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

It's easy to delete multi keys.
In fact, we should determine that how many keys should be actually deleted. This is a read-before-write operation. As we known that Scylla does not support LWT. So should we just read all keys, then delete the exist keys?

@psarna , @nyh can you help to answer @fastio's question?

@fastio , Scylla had supported lwt: #1359

This comment has been minimized.

Copy link
@nyh

nyh Nov 17, 2019

Contributor

This is an interesting question. Indeed Scylla's classic MO is to do writes without read, so you can delete a key without knowing whether it actually existed before or it did not. Redis's DEL documentation also says that it "Removes the specified keys. A key is ignored if it does not exist.", but it is indeed sad that the API also returns the number of keys actually removed. I don't know if users actually use this capability often, but sadly it is officially documented and we'll eventually need to support it.

But we don't need to support it now. What I suggest we do now is to remember that we still have this problem - I suggest doing two things: First have an "xfailing" test for this case (a test deleting a non-existent key expecting the returned integer to be 0) which will succeed on real redis, but fail on Scylla. Second is to create a bug-tracker issue to remember this.

For multiple keys, again, if you don't want to fix this issue immediately, please add an xfailing test and an issue to remember this missing feature. Or if it's trivial, just fix that now.

Eventually, we'll need to decide how to do all sorts of RMW (read-modify-write aka read-before-write) operations that Redis has. Maybe we'll use LWT, maybe some sort of leader model or Raft, or I don't know what; Scylla's DynamoDB support ("Alternator") has the same problems. But we can start without this for now. We just need to remember what we don't support yet, and in my opinion xfailing tests (which succeed on the real redis) is a good way to remember that.

namespace commands {

class del : public abstract_command {
bytes _key;

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

std::vector _keys;

DEL command supports to delete multiple keys once.

```
CREATE TABLE redis (
key text,
value variant\<text, list\<text\>, map\<text\>\>,

This comment has been minimized.

Copy link
@amoskong

amoskong Nov 17, 2019

Contributor

The map only has one item?

This comment has been minimized.

Copy link
@fastio

fastio Nov 17, 2019

Author Contributor

No. This is one of the optional sulotion. As mentioned in the redis.md, this sulotion is not the not better one.

@nyh

This comment has been minimized.

Copy link
Contributor

nyh commented Nov 17, 2019

If we continue to improve this patch set until it absolutely perfect, I think it will be a very long process, and very frustrating for you @fastio to keep rebasing it and maintaining it.

So I want to do something different. I want to commit this work this week; Get this basic support in, and then later you can send incremental patches improving the Redis support. For this, I would like you to please conflate the newer patches in this patch set into the older ones - i.e., I don't want to see a patch like "Redis API: Move the redis transport related codes to redis/service" - I just want the original patch to put it right in the right place immediately. I then want to do one final review over this code which will focus on verifying just one thing: That most of the Redis code is in the redis/ directory, and the amount of changes to other source files is minimal. The reason why I wanted to reach this state (and now to verify we've indeed reached it) is two-fold:

  1. First it reduces the risks of including the new Redis code now - even before it is perfect. If it's very clear that Redis cannot possibly do any harm to users not using it - because the patches don't touch any of the existing Scylla code (or very little of it) - then there is no risk of including the new Redis support now, even before it's 100% ready.

  2. Second, having separate source files will make it easier - and more fun - for you to continue working on improving this Redis support incrementally. This is because your patches will only (or mostly) go to new redis/ files, so the risk that you'll clash with other people's changes will be lower.

Amos has made some good comments in this thread recently, so it would be nice if you could also take them into account for the next (and hopefully final) iteration, but you don't have to fix all of them - you can also create "FIXME"s in the code - and/or bug tracker issues - to remember that these issues exist without fixing them yet.

By the way, the API issues that Amos discovered by running the tests against the "real" redis is exactly why I suggested that we write tests that use the Redis API (and not some internal C++ code) so that they can be run against both our code and against Redis.

@asias

This comment has been minimized.

Copy link
Contributor

asias commented Nov 17, 2019

If we continue to improve this patch set until it absolutely perfect, I think it will be a very long process, and very frustrating for you @fastio to keep rebasing it and maintaining it.

So I want to do something different. I want to commit this work this week; Get this basic support in, and then later you can send incremental patches improving the Redis support.

Nadav, I really liked this approach.

For this, I would like you to please conflate the newer patches in this patch set into the older ones - i.e., I don't want to see a patch like "Redis API: Move the redis transport related codes to redis/service" - I just want the original patch to put it right in the right place immediately. I then want to do one final review over this code which will focus on verifying just one thing: That most of the Redis code is in the redis/ directory, and the amount of changes to other source files is minimal. The reason why I wanted to reach this state (and now to verify we've indeed reached it) is two-fold:

1. First it reduces the **risks** of including the new Redis code now - even before it is perfect. If it's very clear that Redis cannot possibly do any harm to users not using it - because the patches don't touch any of the existing Scylla code (or very little of it) -  then there is no risk of including the new Redis support now, even before it's 100% ready.

2. Second, having separate source files will make it easier - and more fun - for you to continue working on improving this Redis support incrementally. This is because your patches will only (or mostly) go to new redis/ files, so the risk that you'll clash with other people's changes will be lower.

Amos has made some good comments in this thread recently, so it would be nice if you could also take them into account for the next (and hopefully final) iteration, but you don't have to fix all of them - you can also create "FIXME"s in the code - and/or bug tracker issues - to remember that these issues exist without fixing them yet.

By the way, the API issues that Amos discovered by running the tests against the "real" redis is exactly why I suggested that we write tests that use the Redis API (and not some internal C++ code) so that they can be run against both our code and against Redis.

@amoskong

This comment has been minimized.

Copy link
Contributor

amoskong commented Nov 17, 2019

The latest PR only changed configure.py (for building) and db/config.* (for adding new options), and very small & clean change for main.cc for starting/stopping redis service.

The patches are organized a little bit messy ;-) @fastio you can reference my example:
https://github.com/amoskong/scylla/commits/redis-patchset

  • [PATCH 1/5] Document: add docs/redis/redis.md
  • [PATCH 2/5] redis: Redis API in Scylla
  • [PATCH 3/5] Redis API: graft redis module to Scylla
  • [PATCH 4/5] redis-test: add test cases for Redis API
  • [PATCH 5/5] toolchain/Docker: downgrade ragel to 6.8-5

BTW: there are many trailing whitespace in your code, it should be cleaned.

@nyh

This comment has been minimized.

Copy link
Contributor

nyh commented Nov 17, 2019

Yes, @amoskong it's hard to see which files changed when one patch changes one file and a later patch moves it to another file. This can probably be easily cleaned up (like you did). I would really appreciate not having that "toolchain/Docker: downgrade ragel to 6.8-5" patch, though. I want to be able to build with the ragel I have (7.0) - the toolchain should not be mandatory for developers using modern distributions. I'm sure this can be fixed without downgrading Ragel (but I have no idea how - I know nothing about Ragel).

@amoskong

This comment has been minimized.

Copy link
Contributor

amoskong commented Nov 17, 2019

toolchain/dbuild is used for building master packages by our daily jenkins job, the master building will be broken if we don't downgrade ragel to 6.8.

We can revert the patch when if ragel issue is fixed.

@nyh

This comment has been minimized.

Copy link
Contributor

nyh commented Nov 17, 2019

As much as I care about the daily Jenkins job, I also want it to build on my development machine - which uses Ragel 7.0.0.11 (on Fedora 30) and I don't plan to downgrade....

Is this really something we cannot work around in Ragel 7.0 without downgrading? And why doesn't it cause problems in Seastar as well (yes, I see there's an open issue, but I didn't see any problems on my own machine which uses Ragel 7.0, so what am I missing?)

@amoskong

This comment has been minimized.

Copy link
Contributor

amoskong commented Nov 17, 2019

As much as I care about the daily Jenkins job, I also want it to build on my development machine - which uses Ragel 7.0.0.11 (on Fedora 30) and I don't plan to downgrade....

Is this really something we cannot work around in Ragel 7.0 without downgrading? And why doesn't it cause problems in Seastar as well (yes, I see there's an open issue, but I didn't see any problems on my own machine which uses Ragel 7.0, so what am I missing?)

Did you successfully build @fastio 's PR with Ragel 7.0? or just successfully build latest scylla master with this PR?

@fastio fastio force-pushed the fastio:master branch from df691f6 to f05781a Nov 17, 2019
@fastio fastio changed the base branch from master to next Nov 17, 2019
@nyh

This comment has been minimized.

Copy link
Contributor

nyh commented Nov 17, 2019

No, I did not try to compile this pull request. What I meant was that I didn't see any problem with Seastar or its HTTP parser (which I think is where we used ragel?) with the Ragel 7.0 I have in the machine. What is the problem with Ragel 7.0, and how difficult is it to work around it?

@amoskong

This comment has been minimized.

Copy link
Contributor

amoskong commented Nov 18, 2019

No, I did not try to compile this pull request. What I meant was that I didn't see any problem with Seastar or its HTTP parser (which I think is where we used ragel?) with the Ragel 7.0 I have in the machine. What is the problem with Ragel 7.0, and how difficult is it to work around it?

seastar uses fixed version of ragel (6.10 )

$ git grep ragel
CMakeLists.txt:find_package (ragel 6.10 REQUIRED)
CMakeLists.txt:function (seastar_generate_ragel)
CMakeLists.txt:    COMMAND ${ragel_RAGEL_EXECUTABLE} -G2 -o ${args_OUT_FILE} ${args_IN_FILE}
....
cooking_recipe.cmake:    URL http://www.colm.net/files/ragel/ragel-6.10.tar.gz
···

@fastio added an option `--with-ragel`, we can assign different version of ragel.

The only concern is daily jenkins will be broken, this is why I downgrade ragel of dbuild image.
Note: only ragel version of docker image will be downgraded, @nyh it won't downgrade ragel of your machine.
@fastio fastio force-pushed the fastio:master branch from f05781a to f2068c0 Nov 19, 2019
@avikivity

This comment has been minimized.

Copy link
Contributor

avikivity commented Nov 19, 2019

We can't downgrade ragel, the frozen toolchain has to be regenerated from tools/toolchain/Dockerfile, and we can't download random rpms into it.

What's the problem with ragel 7?

@fastio

This comment has been minimized.

Copy link
Contributor Author

fastio commented Nov 19, 2019

@avikivity The problem is that the code generated with ragel 7 contains the in-class initialization of static const integral type members, and these members are never used by the rest codes.

we can provide in-class initializers for static members that have const integral type.

The compiler errors:

build/release/gen/redis/protocol_parser.hh:43:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_targs []' of incomplete type
   43 |  static const char _nfa_targs[] = {
      |                    ^~~~~~~~~~
build/release/gen/redis/protocol_parser.hh:47:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_offsets []' of incomplete type
   47 |  static const char _nfa_offsets[] = {
      |                    ^~~~~~~~~~~~
build/release/gen/redis/protocol_parser.hh:53:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_push_actions []' of incomplete type
   53 |  static const char _nfa_push_actions[] = {
      |                    ^~~~~~~~~~~~~~~~~
build/release/gen/redis/protocol_parser.hh:57:20: error: in-class initialization of static data member 'const char redis_protocol_parser::_nfa_pop_trans []' of incomplete type
   57 |  static const char _nfa_pop_trans[] = {

The code generated by ragel 7:

class redis_protocol_parser : public ragel_parser_base<redis_protocol_parser> {
    
    ...
    
    static const char _nfa_targs[] = { 
        0, 0
    };  
    
    static const char _nfa_offsets[] = { 
        0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0
    };  
    
    static const char _nfa_push_actions[] = { 
        0, 0
    };  
    
    static const char _nfa_pop_trans[] = { 
        0, 0
    };  
    
    ...
};
  

BTW, I found that you mentioned this issue in the ragel-users mail list in 2017, http://www.colm.net/pipermail/ragel-users/2017-June/003481.html

@avikivity

This comment has been minimized.

Copy link
Contributor

avikivity commented Nov 19, 2019

I suppose you can apply the same workaround as in scylladb/seastar@1cb8b0e

@fastio fastio force-pushed the fastio:master branch from f2068c0 to 5082a54 Nov 19, 2019
Copy link
Contributor

nyh left a comment

Leaving comments, I see you just pushed a new version will need to look again to formally approve.

@@ -702,6 +702,20 @@ db::config::config(std::shared_ptr<db::extensions> exts)
"especially when multiple clustering key columns have IN restrictions. Increasing this value can result in server instability.")
, alternator_port(this, "alternator_port", value_status::Used, 0, "Alternator API port")
, alternator_address(this, "alternator_address", value_status::Used, "0.0.0.0", "Alternator API listening address")
, redis_transport_port(this, "redis_transport_port", value_status::Used, 6379, "Port on which the REDIS transport listens for clients.")
, redis_transport_port_ssl(this, "redis_transport_port_ssl", value_status::Used, 9142, "Port on which the REDIS TLS native transport listens for clients.")
, enable_redis_protocol(this, "enable_redis_protocol", value_status::Used, false, "Enable redis protocol; Scylla will process redis protocol as a redis cluster if enable.")

This comment has been minimized.

Copy link
@nyh

nyh Nov 19, 2019

Contributor

I still don't understand what enable_redis_protocol is supposed to do (enable http? https? both? or what) but you can fix this later.

, redis_transport_port_ssl(this, "redis_transport_port_ssl", value_status::Used, 9142, "Port on which the REDIS TLS native transport listens for clients.")
, enable_redis_protocol(this, "enable_redis_protocol", value_status::Used, false, "Enable redis protocol; Scylla will process redis protocol as a redis cluster if enable.")
, redis_read_consistency_level(this, "redis_read_consistency_level", value_status::Used, "ONE", "Consistency level for read operations for redis.")
, redis_write_consistency_level(this, "redis_write_consistency_level", value_status::Used, "ANY", "Consistency level for write operations for redis.")

This comment has been minimized.

Copy link
@nyh

nyh Nov 19, 2019

Contributor

Please remember these review comments, I think you'll still need to address them, but it doesn't need to block committing this version.

CREATE TABLE ZSETs (
pkey text,
ckey double,
data text,

This comment has been minimized.

Copy link
@nyh

nyh Nov 19, 2019

Contributor

Again, I think this is a pretty important point which you didn't address. Please, after this version is committed, do go over all my comments and write down as issues, fixmes, or followup patches, whatever needs to be done.

@nyh
nyh approved these changes Nov 19, 2019
Copy link
Contributor

nyh left a comment

I'm approving this for commit, and will commit it today unless anyone (@avikivity ? @amoskong ?) has an objection. Please speak up now, or forever hold your peace ;-)

@fastio please note that there a still a significant number of comments that I left during my review which you have not addressed. I don't think any of these issues needs to delay the commit - you can address them later - but please do take a look at all of them. I might have got some of my comments wrong, but I think at least some of them are right and worth fixing or at least remembering to fix (using issues in the bug tracker and/or FIXMEs in the code).
Thanks for all your efforts on adding Redis support to Scylla!

@nyh

This comment has been minimized.

Copy link
Contributor

nyh commented Nov 19, 2019

P.S. Don't try to understand the review comments from this "discussion window". Got to the "files changed: 44" link at the top, and there you can see the review comments on the actual lines of code they apply to.

@fastio fastio requested a review from amnonh Nov 19, 2019
configure.py Show resolved Hide resolved
@fastio

This comment has been minimized.

Copy link
Contributor Author

fastio commented Nov 19, 2019

@fastio please note that there a still a significant number of comments that I left during my review which you have not addressed. I don't think any of these issues needs to delay the commit - you can address them later - but please do take a look at all of them. I might have got some of my comments wrong, but I think at least some of them are right and worth fixing or at least remembering to fix (using issues in the bug tracker and/or FIXMEs in the code).

Thanks. I got it. I'll take a look at these issues and fix them soon.

fastio added 4 commits Sep 29, 2019
In this document, the detailed design and implementation of Redis API in
Scylla is provided.

Signed-off-by: Peng Jian <pengjian.uestc@gmail.com>
Scylla has advantage and amazing features. If Redis build on the top of Scylla,
it has the above features automatically. It's achived great progress
in cluster master managment, data persistence, failover and replication.

The benefits to the users are easy to use and develop in their production
environment, and taking avantages of Scylla.

Using the Ragel to parse the Redis request, server abtains the command name
and the parameters from the request, invokes the Scylla's internal API to
read and write the data, then replies to client.

Signed-off-by: Peng Jian, <pengjian.uestc@gmail.com>
In this document, the detailed design and implementation of Redis API in
Scylla is provided.

v2: build: work around ragel 7 generated code bug (suggested by Avi)
    Ragel 7 incorrectly emits some unused variables that don't compile.
    As a workaround, sed them away.

Signed-off-by: Peng Jian <pengjian.uestc@gmail.com>
Signed-off-by: Amos Kong <amos@scylladb.com>
Signed-off-by: Peng Jian <pengjian.uestc@gmail.com>
Signed-off-by: Amos Kong <amos@scylladb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.