sijie commented on a change in pull request #1201: ISSUE #570: Entrylog per
ledger
URL: https://github.com/apache/bookkeeper/pull/1201#discussion_r170564729
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
##########
@@ -588,93 +588,146 @@ public void accept(long ledgerId, long size) {
private long preallocatedLogId;
private Future<BufferedLogChannel> preallocation = null;
private ExecutorService allocatorExecutor;
- private final Object createEntryLogLock = new Object();
- private final Object createCompactionLogLock = new Object();
EntryLoggerAllocator(long logId) {
preallocatedLogId = logId;
allocatorExecutor = Executors.newSingleThreadExecutor();
}
- BufferedLogChannel createNewLog() throws IOException {
- synchronized (createEntryLogLock) {
- BufferedLogChannel bc;
- if (!entryLogPreAllocationEnabled){
- // create a new log directly
- bc = allocateNewLog();
- return bc;
- } else {
- // allocate directly to response request
- if (null == preallocation){
- bc = allocateNewLog();
+ synchronized long getPreallocatedLogId(){
+ return preallocatedLogId;
+ }
+
+ synchronized BufferedLogChannel createNewLog() throws IOException {
+ BufferedLogChannel bc;
+ if (!entryLogPreAllocationEnabled || null == preallocation) {
+ // initialization time to create a new log
+ bc = allocateNewLog();
+ } else {
+ // has a preallocated entry log
+ try {
+ /*
+ * both createNewLog and allocateNewLog are synchronized on
+ * EntryLoggerAllocator.this object. So it is possible that
+ * a thread calling createNewLog would attain the lock on
+ * this object and get to this point but preallocation
+ * Future is starving for lock on EntryLoggerAllocator.this
+ * to execute allocateNewLog. Here since we attained lock
+ * for this it means preallocation future must have either
+ * completed creating new log or still waiting for lock on
+ * this object to execute allocateNewLog method. So we
+ * should try to get result of the future without waiting.
+ * If it fails with TimeoutException then call
+ * allocateNewLog explicitly since we are holding the lock
+ * on this anyhow.
+ *
+ */
+ bc = preallocation.get(0, TimeUnit.MILLISECONDS);
Review comment:
I think this change the behavior, which can cause an entry log is allocated
but never used, no?
The problem why you need to change the behavior, is because you changed to
use `synchronized` here. is the change here really needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
