kezhuw commented on code in PR #2130:
URL: https://github.com/apache/zookeeper/pull/2130#discussion_r1772415584
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -151,7 +158,24 @@ public ZKDatabase(FileTxnSnapLog snapLog) {
DEFAULT_COMMIT_LOG_COUNT);
commitLogCount = DEFAULT_COMMIT_LOG_COUNT;
}
- LOG.info("{}={}", COMMIT_LOG_COUNT, commitLogCount);
+ try {
+ commitLogSize = Double.parseDouble(
Review Comment:
backlog: I think we cloud introduce something similar to
[Time::parseTimeInterval](https://github.com/apache/zookeeper/pull/2178/files#diff-f88e4ed3bd32bdaf079b8114bf957308b9d5dedef10aa65a347888468678ce84R60)
in a later time. The size may look large in case of bytes.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -92,9 +93,16 @@ public class ZKDatabase {
public static final String COMMIT_LOG_COUNT = "zookeeper.commitLogCount";
public static final int DEFAULT_COMMIT_LOG_COUNT = 500;
public int commitLogCount;
+ public static final String COMMIT_LOG_SIZE = "zookeeper.commitLogSize";
+ public static final double DEFAULT_COMMIT_LOG_SIZE =
Runtime.getRuntime().maxMemory() * 0.2;
+ public double commitLogSize;
protected Queue<Proposal> committedLog = new ArrayDeque<>();
protected ReentrantReadWriteLock logLock = new ReentrantReadWriteLock();
private volatile boolean initialized = false;
+ /**
+ * committedLog bytes size.
+ */
+ private AtomicLong totalCommittLogSize = new AtomicLong(0);
Review Comment:
typo: Should be `totalCommitLogSize` ?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -320,17 +345,24 @@ public void addCommittedProposal(Request request) {
WriteLock wl = logLock.writeLock();
try {
wl.lock();
- if (committedLog.size() > commitLogCount) {
- committedLog.remove();
- minCommittedLog = committedLog.peek().getZxid();
- }
if (committedLog.isEmpty()) {
minCommittedLog = request.zxid;
- maxCommittedLog = request.zxid;
}
PureRequestProposal p = new PureRequestProposal(request);
committedLog.add(p);
maxCommittedLog = p.getZxid();
+ totalCommittLogSize.addAndGet(request.getApproximateSize());
+ while (committedLog.size() > 0
Review Comment:
`!committedLog.isEmpty()` ?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -92,9 +93,16 @@ public class ZKDatabase {
public static final String COMMIT_LOG_COUNT = "zookeeper.commitLogCount";
public static final int DEFAULT_COMMIT_LOG_COUNT = 500;
public int commitLogCount;
+ public static final String COMMIT_LOG_SIZE = "zookeeper.commitLogSize";
+ public static final double DEFAULT_COMMIT_LOG_SIZE =
Runtime.getRuntime().maxMemory() * 0.2;
+ public double commitLogSize;
protected Queue<Proposal> committedLog = new ArrayDeque<>();
protected ReentrantReadWriteLock logLock = new ReentrantReadWriteLock();
private volatile boolean initialized = false;
+ /**
+ * committedLog bytes size.
+ */
+ private AtomicLong totalCommittLogSize = new AtomicLong(0);
Review Comment:
And `long` is enough ?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -92,9 +93,16 @@ public class ZKDatabase {
public static final String COMMIT_LOG_COUNT = "zookeeper.commitLogCount";
public static final int DEFAULT_COMMIT_LOG_COUNT = 500;
public int commitLogCount;
+ public static final String COMMIT_LOG_SIZE = "zookeeper.commitLogSize";
Review Comment:
We should document this in `zookeeperAdmin.md`.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -320,17 +345,24 @@ public void addCommittedProposal(Request request) {
WriteLock wl = logLock.writeLock();
try {
wl.lock();
- if (committedLog.size() > commitLogCount) {
- committedLog.remove();
- minCommittedLog = committedLog.peek().getZxid();
- }
if (committedLog.isEmpty()) {
minCommittedLog = request.zxid;
- maxCommittedLog = request.zxid;
}
PureRequestProposal p = new PureRequestProposal(request);
committedLog.add(p);
maxCommittedLog = p.getZxid();
+ totalCommittLogSize.addAndGet(request.getApproximateSize());
+ while (committedLog.size() > 0
+ && (committedLog.size() > commitLogCount ||
totalCommittLogSize.get() > commitLogSize)) {
+ committedLog.remove();
+ Proposal peek = committedLog.peek();
+ if (peek == null) {
+ minCommittedLog = 0;
+ maxCommittedLog = 0;
+ } else {
+ minCommittedLog = p.getZxid();
Review Comment:
Should be `peek.getZxid()` ?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java:
##########
@@ -137,12 +145,11 @@ public ZKDatabase(FileTxnSnapLog snapLog) {
commitLogCount = Integer.parseInt(
System.getProperty(COMMIT_LOG_COUNT,
Integer.toString(DEFAULT_COMMIT_LOG_COUNT)));
- if (commitLogCount < DEFAULT_COMMIT_LOG_COUNT) {
Review Comment:
> The default value is 500 which is the recommended minimum.
(https://zookeeper.apache.org/doc/r3.9.2/zookeeperAdmin.html)
I do think `500` is pretty small for ZooKeeper already. It is meaningless to
support `1` as minimum. Is it better to rename `DEFAULT_COMMIT_LOG_COUNT` to
`MIN_COMMIT_LOG_COUNT` ?
--
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: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
