You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/05/07 07:05:32 UTC
[GitHub] [bookkeeper] merlimat opened a new pull request #2710: Impose a memory limit on the bookie journal
merlimat opened a new pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710
### Motivation
Currently, the journal has a configurable max queue size, with default to 10,000 entries. This can be used as a proxy measure to control the max amount of memory that has to be retained from the entries that are in the journal queue.
This is not very flexible and there are few potential problems that arise:
1. If using multiple journals, each one of them gets 10,000 entries
2. entries can be of very different sizes, from few bytes to several MBs, so it can be difficult to estimate the memory usage
3. if entries are big, it can cause the bookie to go OOM when the queue gets filled up
Instead (or additionally) of number of entries, we should use the total amount of memory as the metric for enabling the backpressure.
### Changes
Added `MemoryLimitController` class, already used in Pulsar (https://github.com/apache/pulsar/pull/8965) and use it to block the thread adding to the journal queue when memory is exhausted.
Defaulting to use 5% of max direct memory (eg: 100MB out of 2GB)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628392118
##########
File path: bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/MemoryLimitController.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.bookkeeper.common.util;
+
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class MemoryLimitController {
+
+ private final long memoryLimit;
+ private final AtomicLong currentUsage = new AtomicLong();
+ private final ReentrantLock mutex = new ReentrantLock(false);
+ private final Condition condition = mutex.newCondition();
+
+ public MemoryLimitController(long memoryLimitBytes) {
+ this.memoryLimit = memoryLimitBytes;
+ }
+
+ public boolean tryReserveMemory(long size) {
+ while (true) {
+ long current = currentUsage.get();
+ long newUsage = current + size;
+
+ // We allow one request to go over the limit, to make the notification
+ // path simpler and more efficient
+ if (current > memoryLimit && memoryLimit > 0) {
Review comment:
Added metrics
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
hsaputra commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628484718
##########
File path: conf/bk_server.conf
##########
@@ -358,6 +358,11 @@ journalDirectories=/tmp/bk-txn
# Set the size of the journal queue.
# journalQueueSize=10000
+# Set the max amount of memory that can be used by the journal.
+# If empty, this will be set to use 5% of available direct memory
+# Setting it to 0, it will disable the max memory control for the journal.
+# journalMaxMemorySizeMb=
Review comment:
So looks like this will be additional limit to t he max Journal Q size?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628342581
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalMaxMemoryTest.java
##########
@@ -0,0 +1,109 @@
+/*
+ *
+ * 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.bookkeeper.bookie;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.bookie.Journal.ForceWriteRequest;
+import org.apache.bookkeeper.bookie.Journal.LastLogMark;
+import org.apache.bookkeeper.common.util.MemoryLimitController;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.conf.TestBKConfiguration;
+import org.apache.bookkeeper.net.BookieId;
+import org.apache.bookkeeper.proto.BookkeeperInternalCallbacks.WriteCallback;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+import org.powermock.reflect.Whitebox;
+import java.io.File;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.TimeUnit;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.powermock.api.mockito.PowerMockito.whenNew;
+
+/**
+ * Test the bookie journal PageCache flush interval.
Review comment:
nit: fix javadoc
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628351661
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalMaxMemoryTest.java
##########
@@ -0,0 +1,109 @@
+/*
+ *
+ * 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.bookkeeper.bookie;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.bookie.Journal.ForceWriteRequest;
+import org.apache.bookkeeper.bookie.Journal.LastLogMark;
+import org.apache.bookkeeper.common.util.MemoryLimitController;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.conf.TestBKConfiguration;
+import org.apache.bookkeeper.net.BookieId;
+import org.apache.bookkeeper.proto.BookkeeperInternalCallbacks.WriteCallback;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+import org.powermock.reflect.Whitebox;
+import java.io.File;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.TimeUnit;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.powermock.api.mockito.PowerMockito.whenNew;
+
+/**
+ * Test the bookie journal PageCache flush interval.
Review comment:
Ups, fixed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#issuecomment-834736891
@dlg99 I marked this on purpose for 4.14
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628507467
##########
File path: conf/bk_server.conf
##########
@@ -358,6 +358,11 @@ journalDirectories=/tmp/bk-txn
# Set the size of the journal queue.
# journalQueueSize=10000
+# Set the max amount of memory that can be used by the journal.
+# If empty, this will be set to use 5% of available direct memory
+# Setting it to 0, it will disable the max memory control for the journal.
+# journalMaxMemorySizeMb=
Review comment:
Correct. that would still be in place
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628351476
##########
File path: bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/MemoryLimitController.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.bookkeeper.common.util;
+
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class MemoryLimitController {
+
+ private final long memoryLimit;
+ private final AtomicLong currentUsage = new AtomicLong();
+ private final ReentrantLock mutex = new ReentrantLock(false);
+ private final Condition condition = mutex.newCondition();
+
+ public MemoryLimitController(long memoryLimitBytes) {
+ this.memoryLimit = memoryLimitBytes;
+ }
+
+ public boolean tryReserveMemory(long size) {
+ while (true) {
+ long current = currentUsage.get();
+ long newUsage = current + size;
+
+ // We allow one request to go over the limit, to make the notification
+ // path simpler and more efficient
+ if (current > memoryLimit && memoryLimit > 0) {
Review comment:
The reason is mostly to leave open the possibility to track how much memory is being used, even if a limit is not being enforced.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat merged pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628349705
##########
File path: bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/MemoryLimitController.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.bookkeeper.common.util;
+
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class MemoryLimitController {
+
+ private final long memoryLimit;
+ private final AtomicLong currentUsage = new AtomicLong();
+ private final ReentrantLock mutex = new ReentrantLock(false);
+ private final Condition condition = mutex.newCondition();
+
+ public MemoryLimitController(long memoryLimitBytes) {
+ this.memoryLimit = memoryLimitBytes;
+ }
+
+ public boolean tryReserveMemory(long size) {
+ while (true) {
+ long current = currentUsage.get();
+ long newUsage = current + size;
+
+ // We allow one request to go over the limit, to make the notification
+ // path simpler and more efficient
+ if (current > memoryLimit && memoryLimit > 0) {
Review comment:
so here if we set memoryLimit = 0 we are not entering this branch
isn't it simpler to move
```
if (memoryLimit <=0) {
return true;
}
```
at the beginning of this method and a similar check in the beginning of `releaseMemory ` ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#issuecomment-834648585
@eolivelli PTAL again
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a change in pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#discussion_r628355394
##########
File path: bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/MemoryLimitController.java
##########
@@ -0,0 +1,84 @@
+/**
+ * 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.bookkeeper.common.util;
+
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class MemoryLimitController {
+
+ private final long memoryLimit;
+ private final AtomicLong currentUsage = new AtomicLong();
+ private final ReentrantLock mutex = new ReentrantLock(false);
+ private final Condition condition = mutex.newCondition();
+
+ public MemoryLimitController(long memoryLimitBytes) {
+ this.memoryLimit = memoryLimitBytes;
+ }
+
+ public boolean tryReserveMemory(long size) {
+ while (true) {
+ long current = currentUsage.get();
+ long newUsage = current + size;
+
+ // We allow one request to go over the limit, to make the notification
+ // path simpler and more efficient
+ if (current > memoryLimit && memoryLimit > 0) {
Review comment:
... which raises the good point that we should have such metric exported :).. I'll quickly add it here
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on pull request #2710: Impose a memory limit on the bookie journal
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #2710:
URL: https://github.com/apache/bookkeeper/pull/2710#issuecomment-834581662
> like configuring -1 as memory limit in order to disable it
As documented in the `bk_server.conf` setting to 0, disables the limit
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org