You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/11/13 03:19:59 UTC
[GitHub] [zookeeper] lanicc opened a new pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
lanicc opened a new pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744
ZKDatabase.txnCount logged non transactional requests
public boolean append(Request si) throws IOException {
txnCount.incrementAndGet();
return this.snapLog.append(si);
}
snaplog.append may return false, but txnCount increased
maybe it would be better
public boolean append(Request si) throws IOException {
if (this.snapLog.append(si)) {
txnCount.incrementAndGet();
return true;
}
return false;
}
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] lanicc commented on a change in pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
lanicc commented on a change in pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#discussion_r748723813
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/TxnLogCountTest.java
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import java.io.File;
+import java.io.IOException;
+import org.apache.jute.OutputArchive;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.txn.TxnHeader;
+import org.junit.Test;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+public class TxnLogCountTest {
+
+ /**
+ * Test ZkDatabase's txnCount
+ */
+ @Test
+ public void testTxnLogCount() throws IOException {
+ File tmpDir = ClientBase.createTmpDir();
+ FileTxnSnapLog snapLog = new FileTxnSnapLog(tmpDir, tmpDir);
Review comment:
Careless！Do I need to submit a patch?
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] asfgit closed pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] lanicc commented on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
lanicc commented on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-962423154
> * @lanicc I am going to merge this PR.
> * Could you please add some unit tests for this change. For example: using the `zk` client to make `m` read requests and `n` write requests, then assert the `txnCount` is equal `n`, not the `m + n`
The unit test has been submitted, PTAL@maoling
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] lanicc commented on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
lanicc commented on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-947213835
> * PTAL @ztzg @eolivelli
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] lanicc removed a comment on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
lanicc removed a comment on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-962417933
> * @lanicc I am going to merge this PR.
> * Could you please add some unit tests for this change. For example: using the `zk` client to make `m` read requests and `n` write requests, then assert the `txnCount` is equal `n`, not the `m + n`
Sorry for my late!I'll finish it in these days.
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-967797730
- @lanicc Thanks for your contribution.
- Commit to the following branches:
[master ](https://github.com/apache/zookeeper/tree/master)
[branch-3.7](https://github.com/apache/zookeeper/tree/branch-3.7)
[branch-3.6](https://github.com/apache/zookeeper/tree/branch-3.6)
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-956163736
- @lanicc I am going to merge this PR.
- Could you please add some unit tests for this change. For example: using the `zk` client to make `m` read requests and `n` write requests, then assert the `txnCount` is equal `n`, not the `m + n`
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on a change in pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#discussion_r748691483
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/server/TxnLogCountTest.java
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.server;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import java.io.File;
+import java.io.IOException;
+import org.apache.jute.OutputArchive;
+import org.apache.zookeeper.server.persistence.FileTxnSnapLog;
+import org.apache.zookeeper.test.ClientBase;
+import org.apache.zookeeper.txn.TxnHeader;
+import org.junit.Test;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+public class TxnLogCountTest {
+
+ /**
+ * Test ZkDatabase's txnCount
+ */
+ @Test
+ public void testTxnLogCount() throws IOException {
+ File tmpDir = ClientBase.createTmpDir();
+ FileTxnSnapLog snapLog = new FileTxnSnapLog(tmpDir, tmpDir);
Review comment:
remember delete this tmpDir next time:)
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-915991022
- @lanicc Great catch.
- Oops, it may be not an improvement, just a hidden bug which was introduced by [PR-770](https://github.com/apache/zookeeper/pull/770)
The refactor is not an equivalence.
```
// track the number of records written to the log
if (zks.getZKDatabase().append(si)) {
logCount++;
if (logCount > randRoll) {
randRoll = ThreadLocalRandom.current().nextInt(snapCount / 2, snapCount);
// roll the log
```
- A worse case: when I do 100000 read requests and then do a write request. In this pattern with a read-heavy use case, too many many snapshots are taken, even the `DataTree` are not changed(if the write request is `closeSession`)
- PTAL @ztzg @eolivelli
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] lanicc commented on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
lanicc commented on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-962417933
> * @lanicc I am going to merge this PR.
> * Could you please add some unit tests for this change. For example: using the `zk` client to make `m` read requests and `n` write requests, then assert the `txnCount` is equal `n`, not the `m + n`
Sorry for my late!I'll finish it in these days.
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling closed pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
maoling closed pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1744: ZOOKEEPER-4362: ZKDatabase.txnCount should not log non transactional requests
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1744:
URL: https://github.com/apache/zookeeper/pull/1744#issuecomment-948516939
- @ztzg @eolivelli @enixon PTAL
- I checked the codes. This issue existed in
[master](https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java)
[branch-3.7](https://github.com/apache/zookeeper/blob/branch-3.7/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java)
[branch-3.6](https://github.com/apache/zookeeper/blob/branch-3.6/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZKDatabase.java)
Lucky, [branch-3.5](https://github.com/apache/zookeeper/blob/branch-3.5/zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java
) did not have this issue.
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org