feat(cmd): add iso8601 time format parameter to LASTSAVE command #2822

SharonIV0x86 · 2025-03-07T16:07:04Z

This closes #2817.

This PR introduces a new parameter to the LASTSAVE command to return the time of last save to disk in a human readable format.

Usage
LASTSAVE ISO8601
LASTSAVE iso8601

LASTSAVE also works for backward compatibility.

127.0.0.1:6666> LASTSAVE
(integer) 1741363334
127.0.0.1:6666> LASTSAVE iso8601
"2025-03-07T21:32:14+0530"
127.0.0.1:6666> LASTSAVE ISO8601
"2025-03-07T21:32:14+0530"
127.0.0.1:6666>

torwig · 2025-03-07T16:28:20Z

src/commands/cmd_server.cc

+      return {Status::RedisParseErr, "unknown extra subcommand"};
+    }
+    if (args.size() == 2) {
+      std::string fmt_arg = args[1];


You can try to use EqualICase (common->string_util.h).

You can try to use EqualICase (common->string_util.h).

Initially i came across parser.EatEqICase() but then i dropped the idea as i didnt know its correct usage so i though i'll take some feedback on its usage.

Thanks for the review i'll use EqualICase as it seems a better fit in this case.

PragmaTwice

It doesn't seem to be a Redis feature. So I'm wondering if it will be useful in production envs?

SharonIV0x86 · 2025-03-08T07:08:05Z

It doesn't seem to be a Redis feature. So I'm wondering if it will be useful in production envs?

You're right this isn't a redis feature.

But instead of converting the UNIX timestamp manually, users can directly retrieve an easily interpretable format.

PragmaTwice · 2025-03-09T09:59:56Z

You're right this isn't a redis feature.

But instead of converting the UNIX timestamp manually, users can directly retrieve an easily interpretable format.

I've checked the issue you opened. I'm fine with this change.

PragmaTwice · 2025-03-09T10:00:58Z

src/commands/cmd_server.cc

 public:
+  Status Parse(const std::vector<std::string> &args) override {
+    if (args.size() > 2) {
+      return {Status::RedisParseErr, "unknown extra subcommand"};


Suggested change

return {Status::RedisParseErr, "unknown extra subcommand"};

return {Status::RedisParseErr, "unknown extra arguments"};

Thanks for review, i was just working on the changes requested by torwig right now, ill make this change as well.

PragmaTwice · 2025-03-09T10:02:38Z

src/commands/cmd_server.cc

+      if (fmt_arg == "iso8601") {
+        format_spec_ = true;
+      } else {
+        return {Status::RedisParseErr, "unknown subcommand"};


Suggested change

return {Status::RedisParseErr, "unknown subcommand"};

return {Status::RedisParseErr, "unknown arguments"};

PragmaTwice · 2025-03-09T10:07:17Z

src/commands/cmd_server.cc

+      std::string fmt_arg = args[1];
+      std::transform(fmt_arg.begin(), fmt_arg.end(), fmt_arg.begin(), ::tolower);
+      if (fmt_arg == "iso8601") {


Suggested change

std::string fmt_arg = args[1];

std::transform(fmt_arg.begin(), fmt_arg.end(), fmt_arg.begin(), ::tolower);

if (fmt_arg == "iso8601") {

if (util::EqualICase(args[1], "iso8601")) {

PragmaTwice · 2025-03-09T10:15:55Z

src/commands/cmd_server.cc

-    *output = redis::Integer(unix_sec);
+    if (format_spec_) {
+      time_t raw_time = static_cast<time_t>(unix_sec);
+      struct tm *local_time = localtime(&raw_time);


We can use the thread-safe version localtime_r.

PragmaTwice · 2025-03-09T11:12:02Z

src/commands/cmd_server.cc

+        strftime(buf, sizeof(buf), "%Y-%m-%dT%H:%M:%S%z", local_time);
+        *output = redis::BulkString(buf);
+      } else {
+        *output = redis::BulkString("Error converting time");


Suggested change

*output = redis::BulkString("Error converting time");

*output = redis::BulkString("Failed to convert timestamp to local time");

src/commands/cmd_server.cc

PragmaTwice

Looks good. Could you add a go test case for your changes?

SharonIV0x86 · 2025-03-10T03:38:21Z

Looks good. Could you add a go test case for your changes?

I am a bit confused about where exactly to add the test for LASTSAVE command, i guess it will make sense to add it in the gocase/unit/server/poll_updates_test.go as the LASTSAVE command is listed under the server section in the docs?

PragmaTwice · 2025-03-10T03:41:54Z

You can add a new go source file under tests/gocase/unit/server/.

src/commands/cmd_server.cc

Update src/commands/cmd_server.cc Co-authored-by: hulk <[email protected]> Update src/commands/cmd_server.cc Update src/commands/cmd_server.cc Update src/commands/cmd_server.cc Co-authored-by: Twice <[email protected]> Update src/commands/cmd_server.cc Co-authored-by: Twice <[email protected]>

git-hulk · 2025-03-10T09:57:13Z

Looks good. @SharonIV0x86 You could use the gofmt to format Go files to pass the CI.

sonarqubecloud · 2025-03-10T16:20:54Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 50%)

See analysis details on SonarQube Cloud

Added iso8601 time format parameter to LASTSAVE command

7e27f75

SharonIV0x86 changed the title ~~enhance: Added iso8601 time format parameter to LASTSAVE command.~~ feat: Added iso8601 time format parameter to LASTSAVE command. Mar 7, 2025

torwig reviewed Mar 7, 2025

View reviewed changes

PragmaTwice reviewed Mar 8, 2025

View reviewed changes

PragmaTwice reviewed Mar 9, 2025

View reviewed changes

Made suggested changes

27449d3

SharonIV0x86 requested review from PragmaTwice and torwig March 9, 2025 15:53

git-hulk reviewed Mar 10, 2025

View reviewed changes