You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "XuQianJin-Stars (via GitHub)" <gi...@apache.org> on 2023/07/18 13:20:13 UTC
[GitHub] [bookkeeper] XuQianJin-Stars opened a new pull request, #4035: Run table service as a bookkeeper lifecycle component in bookie server
XuQianJin-Stars opened a new pull request, #4035:
URL: https://github.com/apache/bookkeeper/pull/4035
### Motivation
Run table service as a bookkeeper lifecycle component in bookie server.
### Changes
Add `TABLE_SERVICE_ENABLED` parameter in `EmbeddedServer` to control the startup of table service.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] XuQianJin-Stars commented on a diff in pull request #4035: Run table service as a bookkeeper lifecycle component in bookie server
Posted by "XuQianJin-Stars (via GitHub)" <gi...@apache.org>.
XuQianJin-Stars commented on code in PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#discussion_r1266820629
##########
tools/perf/src/main/java/org/apache/bookkeeper/tools/perf/table/PerfClient.java:
##########
@@ -106,7 +106,7 @@ public static class Flags extends CliFlags {
"-nk", "--num-keys"
},
description = "Number of the keys to test")
- public int numKeys = 1000000;
+ public long numKeys = 1000000L;
Review Comment:
> Why do we need this change?
run this cli command can throw error.
```
bin/bkperf 127.0.0.1:4181 table benchmark -mor 1000 -ns ns1 -t t_fill_seq -b fillseq -nk 10000000000 -no 1000
```
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] XuQianJin-Stars commented on a diff in pull request #4035: Run table service as a bookkeeper lifecycle component in bookie server
Posted by "XuQianJin-Stars (via GitHub)" <gi...@apache.org>.
XuQianJin-Stars commented on code in PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#discussion_r1268840273
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java:
##########
@@ -4102,4 +4109,113 @@ public ServerConfiguration setLedgerMetadataRocksdbConf(String ledgerMetadataRoc
this.setProperty(LEDGER_METADATA_ROCKSDB_CONF, ledgerMetadataRocksdbConf);
return this;
}
+
+ /**
+ * Returns the number of bookies to run in this cluster.
+ *
+ * @return the number of bookies to run in this cluster.
+ */
+ public int getNumBookies() {
+ return getInt(TABLE_NUM_BOOKIES, 1);
+ }
+
+ /**
+ * Set the number of bookies to run in this cluster.
+ *
+ * @param numBookies the number of bookies
+ * @return ServerConfiguration
+ */
+ public ServerConfiguration setNumBookies(int numBookies) {
+ setProperty(TABLE_NUM_BOOKIES, numBookies);
+ return this;
+ }
+
+ /**
+ * Returns if should start zookeeper.
+ *
+ * @return true if should start zookeeper, otherwise false.
+ */
+ public boolean getShouldStartZooKeeper() throws ConfigurationException {
Review Comment:
> Why do we need this one?
use in `StandaloneStarter` to start zookeeper.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] XuQianJin-Stars commented on pull request #4035: Run table service as a bookkeeper lifecycle component in bookie server
Posted by "XuQianJin-Stars (via GitHub)" <gi...@apache.org>.
XuQianJin-Stars commented on PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#issuecomment-1641274848
> The bookkeeper provides a configuration named `extraServerComponents` to allow the user to load extra ServerLifecycleComponent. Why not use this way?
Well, it can be specified in extraServerComponents. It’s just that I was hesitant to release it separately and modify it later.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] XuQianJin-Stars commented on a diff in pull request #4035: Run table service as a bookkeeper lifecycle component in bookie server
Posted by "XuQianJin-Stars (via GitHub)" <gi...@apache.org>.
XuQianJin-Stars commented on code in PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#discussion_r1266823934
##########
conf/bk_server.conf:
##########
@@ -1080,14 +1089,16 @@ dlog.bkcAckQuorumSize=2
### Storage ###
# local storage directories for storing table ranges data (e.g. rocksdb sst files)
-storage.range.store.dirs=data/bookkeeper/ranges
+storage.range.store.dirs=/tmp/bk-ranges
Review Comment:
> Is this needed for this PR? Would you mind keeping it as original?
well, can keeping it as original.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] jiazhai commented on a diff in pull request #4035: Run table service as a bookkeeper lifecycle component in bookie server
Posted by "jiazhai (via GitHub)" <gi...@apache.org>.
jiazhai commented on code in PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#discussion_r1266801044
##########
stream/server/src/main/java/org/apache/bookkeeper/stream/cluster/StreamCluster.java:
##########
@@ -54,23 +42,36 @@
import org.apache.commons.lang3.tuple.Pair;
import org.apache.distributedlog.LocalDLMEmulator;
+import java.io.File;
Review Comment:
seems most of these changes are not needed.
##########
conf/bk_server.conf:
##########
@@ -1065,10 +1065,19 @@ zkEnableSecurity=false
##################################################################
##################################################################
+# The flag enables/disables starting the table service. Default value is 'false'.
+tableServiceEnabled=true
Review Comment:
The default value should be false?
##########
tools/perf/src/main/java/org/apache/bookkeeper/tools/perf/table/PerfClient.java:
##########
@@ -106,7 +106,7 @@ public static class Flags extends CliFlags {
"-nk", "--num-keys"
},
description = "Number of the keys to test")
- public int numKeys = 1000000;
+ public long numKeys = 1000000L;
Review Comment:
Why do we need this change?
##########
stream/server/src/main/java/org/apache/bookkeeper/stream/cluster/StreamCluster.java:
##########
@@ -92,37 +94,38 @@ private static ServerConfiguration newBookieConfiguration(ServiceURI serviceURI)
private int nextBookiePort;
private int nextGrpcPort;
- private StreamCluster(StreamClusterSpec spec) {
+ public StreamCluster(ServerConfiguration serverConf) {
Review Comment:
We should keep the original "private" instead of making it "public".
##########
conf/bk_server.conf:
##########
@@ -1080,14 +1089,16 @@ dlog.bkcAckQuorumSize=2
### Storage ###
# local storage directories for storing table ranges data (e.g. rocksdb sst files)
-storage.range.store.dirs=data/bookkeeper/ranges
+storage.range.store.dirs=/tmp/bk-ranges
Review Comment:
Is this needed for this PR? Would you mind keeping it as original?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #4035: Run table service as a bookkeeper lifecycle component in bookie server
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#discussion_r1268158517
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java:
##########
@@ -279,6 +279,13 @@ public class ServerConfiguration extends AbstractConfiguration<ServerConfigurati
protected static final String HTTP_SERVER_TRUST_STORE_PATH = "httpServerTrustStorePath";
protected static final String HTTP_SERVER_TRUST_STORE_PASSWORD = "httpServerTrustStorePassword";
+ // Table Service parameters
+ protected static final String TABLE_NUM_BOOKIES = "tableNumBookies";
Review Comment:
The table service run within the Bookie, why do we need to set tableNumBookies?
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java:
##########
@@ -4102,4 +4109,113 @@ public ServerConfiguration setLedgerMetadataRocksdbConf(String ledgerMetadataRoc
this.setProperty(LEDGER_METADATA_ROCKSDB_CONF, ledgerMetadataRocksdbConf);
return this;
}
+
+ /**
+ * Returns the number of bookies to run in this cluster.
+ *
+ * @return the number of bookies to run in this cluster.
+ */
+ public int getNumBookies() {
+ return getInt(TABLE_NUM_BOOKIES, 1);
+ }
+
+ /**
+ * Set the number of bookies to run in this cluster.
+ *
+ * @param numBookies the number of bookies
+ * @return ServerConfiguration
+ */
+ public ServerConfiguration setNumBookies(int numBookies) {
+ setProperty(TABLE_NUM_BOOKIES, numBookies);
+ return this;
+ }
+
+ /**
+ * Returns if should start zookeeper.
+ *
+ * @return true if should start zookeeper, otherwise false.
+ */
+ public boolean getShouldStartZooKeeper() throws ConfigurationException {
+ return getMetadataServiceUri() == null;
+ }
+
+ /**
+ * Returns the zookeeper server port used in this cluster.
+ *
+ * @return the zookeeper server port used in this cluster.
+ */
+ public int getZKPort() {
Review Comment:
The table service shares the same Zookeeper with the Bookie, why do we set the zookeeper port?
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java:
##########
@@ -4102,4 +4109,113 @@ public ServerConfiguration setLedgerMetadataRocksdbConf(String ledgerMetadataRoc
this.setProperty(LEDGER_METADATA_ROCKSDB_CONF, ledgerMetadataRocksdbConf);
return this;
}
+
+ /**
+ * Returns the number of bookies to run in this cluster.
+ *
+ * @return the number of bookies to run in this cluster.
+ */
+ public int getNumBookies() {
+ return getInt(TABLE_NUM_BOOKIES, 1);
+ }
+
+ /**
+ * Set the number of bookies to run in this cluster.
+ *
+ * @param numBookies the number of bookies
+ * @return ServerConfiguration
+ */
+ public ServerConfiguration setNumBookies(int numBookies) {
+ setProperty(TABLE_NUM_BOOKIES, numBookies);
+ return this;
+ }
+
+ /**
+ * Returns if should start zookeeper.
+ *
+ * @return true if should start zookeeper, otherwise false.
+ */
+ public boolean getShouldStartZooKeeper() throws ConfigurationException {
Review Comment:
Why do we need this one?
##########
conf/standalone.conf:
##########
@@ -23,15 +23,68 @@
#bookieId=
#allowLoopback=false
+############################################## Bookie Storage ##############################################
Review Comment:
Please remove the un-related configurations
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java:
##########
@@ -279,6 +279,13 @@ public class ServerConfiguration extends AbstractConfiguration<ServerConfigurati
protected static final String HTTP_SERVER_TRUST_STORE_PATH = "httpServerTrustStorePath";
protected static final String HTTP_SERVER_TRUST_STORE_PASSWORD = "httpServerTrustStorePassword";
+ // Table Service parameters
+ protected static final String TABLE_NUM_BOOKIES = "tableNumBookies";
+ protected static final String TABLE_ZK_PORT = "tableZKPort";
+ protected static final String TABLE_START_BOOKIE_AND_START_PROVIDER = "startBookieAndStartProvider";
Review Comment:
Can we simplify the parameter to `enableTableService`?
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java:
##########
@@ -4102,4 +4109,113 @@ public ServerConfiguration setLedgerMetadataRocksdbConf(String ledgerMetadataRoc
this.setProperty(LEDGER_METADATA_ROCKSDB_CONF, ledgerMetadataRocksdbConf);
return this;
}
+
+ /**
+ * Returns the number of bookies to run in this cluster.
+ *
+ * @return the number of bookies to run in this cluster.
+ */
+ public int getNumBookies() {
+ return getInt(TABLE_NUM_BOOKIES, 1);
+ }
+
+ /**
+ * Set the number of bookies to run in this cluster.
+ *
+ * @param numBookies the number of bookies
+ * @return ServerConfiguration
+ */
+ public ServerConfiguration setNumBookies(int numBookies) {
+ setProperty(TABLE_NUM_BOOKIES, numBookies);
+ return this;
+ }
+
+ /**
+ * Returns if should start zookeeper.
+ *
+ * @return true if should start zookeeper, otherwise false.
+ */
+ public boolean getShouldStartZooKeeper() throws ConfigurationException {
+ return getMetadataServiceUri() == null;
+ }
+
+ /**
+ * Returns the zookeeper server port used in this cluster.
+ *
+ * @return the zookeeper server port used in this cluster.
+ */
+ public int getZKPort() {
+ return getInt(TABLE_ZK_PORT, 2181);
+ }
+
+ /**
+ * Set the zookeeper server port used in this cluster.
+ *
+ * @param zkPort zookeeper server port
+ * @return ServerConfiguration
+ */
+ public ServerConfiguration setZKPort(int zkPort) {
+ setProperty(TABLE_ZK_PORT, zkPort);
+ return this;
+ }
+
+ /**
+ * Returns the gRPC server port used in this cluster.
+ *
+ * @return the gRPC server port used in this cluster.
+ */
+ public int getInitialBookieGrpcPort() {
+ return getInt(TABLE_INITIAL_GRPC_PORT, 4181);
+ }
+
+ /**
+ * Set the gRPC server port used in this cluster.
+ *
+ * @param initialBookieGrpcPort gRPC server port
+ * @return ServerConfiguration
+ */
+ public ServerConfiguration setInitialBookieGrpcPort(int initialBookieGrpcPort) {
+ setProperty(TABLE_INITIAL_GRPC_PORT, initialBookieGrpcPort);
+ return this;
+ }
+
+ /**
+ * The local storage directories for storing table ranges data (e.g. rocksdb sst files).
+ *
+ * @return local storage directories for storing table ranges data.
+ */
+ public String getStorageRangeStoreDirs() {
Review Comment:
Can we share the same directory with ledgers?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #4035: Run table service as a bookkeeper lifecycle component in bookie server
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#issuecomment-1642231693
@diegosalvi @Reidddddd Please help take a look at this Pr, thanks.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Run table service as a bookkeeper lifecycle component in bookie server [bookkeeper]
Posted by "liangyuanpeng (via GitHub)" <gi...@apache.org>.
liangyuanpeng commented on PR #4035:
URL: https://github.com/apache/bookkeeper/pull/4035#issuecomment-2566558984
@XuQianJin-Stars seems like it would be useful, do you want to pick it up?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org