You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/10/14 09:22:48 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #989: Refactor source file structure
PragmaTwice opened a new pull request, #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989
Mostly follows https://github.com/apache/incubator-kvrocks/issues/977#issuecomment-1278446623, except:
- `log_collector` and `tls_util` is moved to `network/`
- add `task_runner` to `common/`
--
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] [incubator-kvrocks] caipengbo commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278778550
> log_collector is not a network-related thing, it is a statistical thing, how about adding a stats directory with log_collector.h and stats.h or even redis_disk.h (or can rename it disk_stats.h?)
We renamed `redis_disk.h` to `disk_stats.h`, do you think that is reasonable? @tanruixiang
--
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] [incubator-kvrocks] tisonkun commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995598677
##########
src/cluster/cluster.cc:
##########
@@ -25,10 +25,10 @@
#include <cstring>
#include <memory>
+#include "commands/redis_cmd.h"
Review Comment:
> I think we need to unify the style of filenames, if it is Command, maybe it will be Command/RedisCmd.h, so I think we can keep underscore-style.
Make sense :)
--
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] [incubator-kvrocks] tanruixiang commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278822247
> We renamed `redis_disk.h` to `disk_stats.h`, do you think that is reasonable? @tanruixiang
Yes, I think it's reasonable.
--
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] [incubator-kvrocks] caipengbo commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278774912
`geohash.h` file appears to be used only by `redis_geo.h`. I don't think it's appropriate to put it in `storage`. Can we just put it inside `types`?
--
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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278775630
> `geohash.h` file appears to be used only by `redis_geo.h`. I don't think it's appropriate to put it in `storage`. Can we just put it inside `types`?
Agree with that.
--
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] [incubator-kvrocks] tisonkun commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r996302285
##########
src/commands/redis_cmd.cc:
##########
@@ -32,30 +32,30 @@
#include <unordered_map>
Review Comment:
I'd recall naming this folder as `command`. What do you think?
--
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] [incubator-kvrocks] tisonkun merged pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989
--
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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1279984864
> @PragmaTwice please rebase on the latest unstable branch.
Done.
--
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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1279679313
> The current diff in github review page seems to be a mess, maybe because I rebase some commits.
Solved by re-rebase and force-push.
--
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] [incubator-kvrocks] caipengbo commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278760233
@PragmaTwice log_collector is not a network-related thing, it is a statistical thing, how about adding a `stats` directory with `log_collector.h` and `stats.h` or even `redis_disk.h` (or can rename it `disk_stats.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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278775001
Done. Current structure:
```
$ tree src
├── cluster
│ ├── cluster.cc
│ ├── cluster.h
│ ├── redis_slot.cc
│ ├── redis_slot.h
│ ├── replication.cc
│ ├── replication.h
│ ├── slot_import.cc
│ ├── slot_import.h
│ ├── slot_migrate.cc
│ └── slot_migrate.h
├── commands
│ ├── redis_cmd.cc
│ └── redis_cmd.h
├── common
│ ├── cron.cc
│ ├── cron.h
│ ├── db_util.h
│ ├── encoding.cc
│ ├── encoding.h
│ ├── event_util.h
│ ├── fd_util.h
│ ├── parse_util.h
│ ├── rand.cc
│ ├── rand.h
│ ├── rocksdb_crc32c.h
│ ├── rw_lock.h
│ ├── scope_exit.h
│ ├── sha1.cc
│ ├── sha1.h
│ ├── status.h
│ ├── task_runner.cc
│ ├── task_runner.h
│ ├── util.cc
│ └── util.h
├── config
│ ├── config.cc
│ ├── config.h
│ ├── config_type.h
│ ├── config_util.cc
│ └── config_util.h
├── main.cc
├── network
│ ├── redis_connection.cc
│ ├── redis_connection.h
│ ├── redis_reply.cc
│ ├── redis_reply.h
│ ├── redis_request.cc
│ ├── redis_request.h
│ ├── server.cc
│ ├── server.h
│ ├── tls_util.cc
│ ├── tls_util.h
│ ├── worker.cc
│ └── worker.h
├── stats
│ ├── disk_stats.cc
│ ├── disk_stats.h
│ ├── log_collector.cc
│ ├── log_collector.h
│ ├── stats.cc
│ └── stats.h
├── storage
│ ├── batch_extractor.cc
│ ├── batch_extractor.h
│ ├── compact_filter.cc
│ ├── compact_filter.h
│ ├── compaction_checker.cc
│ ├── compaction_checker.h
│ ├── event_listener.cc
│ ├── event_listener.h
│ ├── geohash.cc
│ ├── geohash.h
│ ├── lock_manager.cc
│ ├── lock_manager.h
│ ├── redis_db.cc
│ ├── redis_db.h
│ ├── redis_metadata.cc
│ ├── redis_metadata.h
│ ├── redis_pubsub.cc
│ ├── redis_pubsub.h
│ ├── scripting.cc
│ ├── scripting.h
│ ├── storage.cc
│ ├── storage.h
│ ├── table_properties_collector.cc
│ └── table_properties_collector.h
├── types
│ ├── redis_bitmap.cc
│ ├── redis_bitmap.h
│ ├── redis_bitmap_string.cc
│ ├── redis_bitmap_string.h
│ ├── redis_geo.cc
│ ├── redis_geo.h
│ ├── redis_hash.cc
│ ├── redis_hash.h
│ ├── redis_list.cc
│ ├── redis_list.h
│ ├── redis_set.cc
│ ├── redis_set.h
│ ├── redis_sortedint.cc
│ ├── redis_sortedint.h
│ ├── redis_stream_base.cc
│ ├── redis_stream_base.h
│ ├── redis_stream.cc
│ ├── redis_stream.h
│ ├── redis_string.cc
│ ├── redis_string.h
│ ├── redis_zset.cc
│ └── redis_zset.h
├── valgrind.sup
└── version.h.in
8 directories, 104 files
```
--
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] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995600912
##########
CMakeLists.txt:
##########
@@ -148,12 +148,12 @@ find_library(FOUND_UNWIND_LIB unwind)
set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
# kvrocks objects target
-file(GLOB KVROCKS_SRCS src/*.cc)
+file(GLOB_RECURSE KVROCKS_SRCS src/*.cc)
Review Comment:
I think there is not much dependency between translation units. In theory, if there are 1000 source files, a 1000-core cpu and enough memory, we can compile completely in parallel.
--
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] [incubator-kvrocks] git-hulk commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995601672
##########
src/cluster/cluster.cc:
##########
@@ -25,10 +25,10 @@
#include <cstring>
#include <memory>
+#include "commands/redis_cmd.h"
Review Comment:
> Does C++ module style prefer UpperCase? That is, "Command/redis_cmd.h".
ClickHouse src dir uses uppercase but some of dir is the snake case, I can't say which one is better, but I prefer the snake case than the uppercase from my own habits.
--
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] [incubator-kvrocks] torwig commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r996302497
##########
src/commands/redis_cmd.cc:
##########
@@ -32,30 +32,30 @@
#include <unordered_map>
Review Comment:
@tisonkun Do you mean singular `command` instead of plural `commands`? If yes, then we have `types`, `tools` and `tests`, so `commands` is OK in my opinion.
--
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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1279656272
> @PragmaTwice After looking detail, it seems `server` is better than `network` to replace `service`. It's my bad that didn't mention this option, others are good to me. I think we can merge it as soon as possible to avoid conflicts.
Done.
--
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] [incubator-kvrocks] tisonkun commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r996308689
##########
src/commands/redis_cmd.cc:
##########
@@ -32,30 +32,30 @@
#include <unordered_map>
Review Comment:
@torwig well. Since Kvrocks code is almost internal, that users talk to the server via wire protocol, I'm OK with either choice.
--
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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278756642
> `log_collector` is not a network thing, and i think it should be put under `common`
I think both `common` and `network` is ok, I put it into `network` because I find that this file relies on `redis_reply` and is using redis protocol. I will move it to `common`.
--
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] [incubator-kvrocks] caipengbo commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278745612
`log_collector` is not a network thing, and i think it should be put under `common`
--
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] [incubator-kvrocks] tisonkun commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995591735
##########
CMakeLists.txt:
##########
@@ -148,12 +148,12 @@ find_library(FOUND_UNWIND_LIB unwind)
set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
# kvrocks objects target
-file(GLOB KVROCKS_SRCS src/*.cc)
+file(GLOB_RECURSE KVROCKS_SRCS src/*.cc)
Review Comment:
Does the build benefit from separating compile units? For example, one `.o` per directory and for better incremental compilation. If it does we can implement it here or as a follow-up.
##########
CMakeLists.txt:
##########
@@ -148,12 +148,12 @@ find_library(FOUND_UNWIND_LIB unwind)
set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
# kvrocks objects target
-file(GLOB KVROCKS_SRCS src/*.cc)
+file(GLOB_RECURSE KVROCKS_SRCS src/*.cc)
list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
-target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_include_directories(kvrocks_objs PUBLIC src src/common ${PROJECT_BINARY_DIR})
Review Comment:
What's the reason of this change?
##########
src/cluster/cluster.cc:
##########
@@ -25,10 +25,10 @@
#include <cstring>
#include <memory>
+#include "commands/redis_cmd.h"
Review Comment:
Besides, prefer "command" to "commands".
##########
src/cluster/cluster.cc:
##########
@@ -25,10 +25,10 @@
#include <cstring>
#include <memory>
+#include "commands/redis_cmd.h"
Review Comment:
Does C++ module style prefer UpperCase? That is, "Command/redis_cmd.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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278782821
Current source structure:
```
$ tree src
├── cluster
│ ├── cluster.cc
│ ├── cluster.h
│ ├── redis_slot.cc
│ ├── redis_slot.h
│ ├── replication.cc
│ ├── replication.h
│ ├── slot_import.cc
│ ├── slot_import.h
│ ├── slot_migrate.cc
│ └── slot_migrate.h
├── commands
│ ├── redis_cmd.cc
│ └── redis_cmd.h
├── common
│ ├── cron.cc
│ ├── cron.h
│ ├── db_util.h
│ ├── encoding.cc
│ ├── encoding.h
│ ├── event_util.h
│ ├── fd_util.h
│ ├── parse_util.h
│ ├── rand.cc
│ ├── rand.h
│ ├── rocksdb_crc32c.h
│ ├── rw_lock.h
│ ├── scope_exit.h
│ ├── sha1.cc
│ ├── sha1.h
│ ├── status.h
│ ├── task_runner.cc
│ ├── task_runner.h
│ ├── util.cc
│ └── util.h
├── config
│ ├── config.cc
│ ├── config.h
│ ├── config_type.h
│ ├── config_util.cc
│ └── config_util.h
├── main.cc
├── network
│ ├── redis_connection.cc
│ ├── redis_connection.h
│ ├── redis_reply.cc
│ ├── redis_reply.h
│ ├── redis_request.cc
│ ├── redis_request.h
│ ├── server.cc
│ ├── server.h
│ ├── tls_util.cc
│ ├── tls_util.h
│ ├── worker.cc
│ └── worker.h
├── stats
│ ├── disk_stats.cc
│ ├── disk_stats.h
│ ├── log_collector.cc
│ ├── log_collector.h
│ ├── stats.cc
│ └── stats.h
├── storage
│ ├── batch_extractor.cc
│ ├── batch_extractor.h
│ ├── compact_filter.cc
│ ├── compact_filter.h
│ ├── compaction_checker.cc
│ ├── compaction_checker.h
│ ├── event_listener.cc
│ ├── event_listener.h
│ ├── lock_manager.cc
│ ├── lock_manager.h
│ ├── redis_db.cc
│ ├── redis_db.h
│ ├── redis_metadata.cc
│ ├── redis_metadata.h
│ ├── redis_pubsub.cc
│ ├── redis_pubsub.h
│ ├── scripting.cc
│ ├── scripting.h
│ ├── storage.cc
│ ├── storage.h
│ ├── table_properties_collector.cc
│ └── table_properties_collector.h
├── types
│ ├── geohash.cc
│ ├── geohash.h
│ ├── redis_bitmap.cc
│ ├── redis_bitmap.h
│ ├── redis_bitmap_string.cc
│ ├── redis_bitmap_string.h
│ ├── redis_geo.cc
│ ├── redis_geo.h
│ ├── redis_hash.cc
│ ├── redis_hash.h
│ ├── redis_list.cc
│ ├── redis_list.h
│ ├── redis_set.cc
│ ├── redis_set.h
│ ├── redis_sortedint.cc
│ ├── redis_sortedint.h
│ ├── redis_stream_base.cc
│ ├── redis_stream_base.h
│ ├── redis_stream.cc
│ ├── redis_stream.h
│ ├── redis_string.cc
│ ├── redis_string.h
│ ├── redis_zset.cc
│ └── redis_zset.h
├── valgrind.sup
└── version.h.in
8 directories, 104 files
```
--
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] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995597540
##########
src/cluster/cluster.cc:
##########
@@ -25,10 +25,10 @@
#include <cstring>
#include <memory>
+#include "commands/redis_cmd.h"
Review Comment:
I think we need to unify the style of filenames, if it is `Command`, maybe it will be `Command/RedisCmd.h`, so I think we can keep underscore-style.
--
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] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995598798
##########
CMakeLists.txt:
##########
@@ -148,12 +148,12 @@ find_library(FOUND_UNWIND_LIB unwind)
set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
# kvrocks objects target
-file(GLOB KVROCKS_SRCS src/*.cc)
+file(GLOB_RECURSE KVROCKS_SRCS src/*.cc)
list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
-target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_include_directories(kvrocks_objs PUBLIC src src/common ${PROJECT_BINARY_DIR})
Review Comment:
Since header files in `common` is frequently used, so e.g, it can be just `"status.h"` rather than `"common/status.h"`.
##########
CMakeLists.txt:
##########
@@ -148,12 +148,12 @@ find_library(FOUND_UNWIND_LIB unwind)
set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
# kvrocks objects target
-file(GLOB KVROCKS_SRCS src/*.cc)
+file(GLOB_RECURSE KVROCKS_SRCS src/*.cc)
list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
-target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_include_directories(kvrocks_objs PUBLIC src src/common ${PROJECT_BINARY_DIR})
Review Comment:
Since header files in `common` is frequently used, so e.g. it can be just `"status.h"` rather than `"common/status.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] [incubator-kvrocks] git-hulk commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995601672
##########
src/cluster/cluster.cc:
##########
@@ -25,10 +25,10 @@
#include <cstring>
#include <memory>
+#include "commands/redis_cmd.h"
Review Comment:
> Does C++ module style prefer UpperCase? That is, "Command/redis_cmd.h".
ClickHouse src dir uses uppercase but some of dir is the snake case. I can't say which one is better, but I prefer the snake case than the uppercase from my own habits.
--
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] [incubator-kvrocks] git-hulk commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1279217143
@PragmaTwice After looking detail, it seems `server` is better than `network` to replace `service`. It's my bad that didn't mention this option, others are good to me. I think we can merge it as soon as possible to avoid conflicts.
--
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] [incubator-kvrocks] torwig commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r996302497
##########
src/commands/redis_cmd.cc:
##########
@@ -32,30 +32,30 @@
#include <unordered_map>
Review Comment:
Do you mean singular `command` instead of plural `commands`?
--
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] [incubator-kvrocks] tisonkun commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1279901174
@PragmaTwice please rebase on the latest unstable branch.
@ShooterIT @caipengbo could you give a review on this patch? Perhaps we should move it forward in a timely manner to avoid further conflicts.
--
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] [incubator-kvrocks] PragmaTwice commented on pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#issuecomment-1278761769
> @PragmaTwice log_collector is not a network-related thing, it is a statistical thing, how about adding a `stats` directory with `log_collector.h` and `stats.h` or even `redis_disk.h` (or can rename it `disk_stats.h`?)
Greate idea. I will do it.
--
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] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #989: Refactor source file structure
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #989:
URL: https://github.com/apache/incubator-kvrocks/pull/989#discussion_r995598798
##########
CMakeLists.txt:
##########
@@ -148,12 +148,12 @@ find_library(FOUND_UNWIND_LIB unwind)
set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)
# kvrocks objects target
-file(GLOB KVROCKS_SRCS src/*.cc)
+file(GLOB_RECURSE KVROCKS_SRCS src/*.cc)
list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)
add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})
-target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_include_directories(kvrocks_objs PUBLIC src src/common ${PROJECT_BINARY_DIR})
Review Comment:
Since header files in `common` is frequently used, so i.e. it can be just `"status.h"` rather than `"common/status.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