Skip to content
Permalink
Browse files
[build] Fix various spotbugs warnings (#3160)
  • Loading branch information
nicoloboschi committed Apr 1, 2022
1 parent 4443c60 commit 62cfc0ec8670cb66ce4bf2b0d707f42cba5e3354
Showing 49 changed files with 216 additions and 148 deletions.
@@ -50,7 +50,7 @@ jobs:
with:
java-version: 1.8
- name: Build with Maven
run: mvn clean package -B -nsu -DskipBookKeeperServerTests -Dorg.slf4j.simpleLogger.defaultLogLevel=INFO
run: mvn clean package spotbugs:check -B -nsu -DskipBookKeeperServerTests -Dorg.slf4j.simpleLogger.defaultLogLevel=INFO

- name: print JVM thread dumps when cancelled
if: cancelled()
@@ -52,7 +52,7 @@ jobs:
distribution: 'temurin'
java-version: 11
- name: Validate pull request
run: mvn clean -B -nsu apache-rat:check checkstyle:check package -Ddistributedlog -DskipTests -Dorg.slf4j.simpleLogger.defaultLogLevel=INFO
run: mvn clean -B -nsu apache-rat:check checkstyle:check spotbugs:check package -Ddistributedlog -DskipTests -Dorg.slf4j.simpleLogger.defaultLogLevel=INFO

- name: Check license files
run: dev/check-all-licenses
@@ -21,6 +21,7 @@

import static java.nio.charset.StandardCharsets.UTF_8;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.Enumeration;
@@ -147,6 +148,7 @@ private static void usage(Options options) {
}

@SuppressWarnings("deprecation")
@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE")
public static void main(String[] args) throws Exception {
Options options = new Options();
options.addOption("ledger", true, "Ledger to read. If empty, read all ledgers which come available. "
@@ -28,6 +28,7 @@ dependencies {
implementation depLibs.commonsIO

compileOnly depLibs.lombok
compileOnly depLibs.spotbugsAnnotations
annotationProcessor depLibs.lombok

testImplementation depLibs.junit
@@ -53,5 +53,4 @@
<scope>test</scope>
</dependency>
</dependencies>

</project>
@@ -20,6 +20,7 @@
*/
package org.apache.bookkeeper.http.servlet;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.IOException;
import java.io.Writer;
import java.util.Enumeration;
@@ -69,6 +70,7 @@ public void bindHandler(String endpoint, HttpServer.ApiType mapping) {
}

@Override
@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE")
protected void service(HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws IOException {
HttpServiceRequest request = new HttpServiceRequest()
.setMethod(httpServerMethod(httpRequest))
@@ -24,6 +24,7 @@
import static org.apache.bookkeeper.util.BookKeeperConstants.METADATA_CACHE;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import io.netty.util.concurrent.DefaultThreadFactory;

import java.io.IOException;
@@ -594,6 +595,7 @@ int calculateUsageIndex(int numBuckets, double usage) {
*
* @throws InterruptedException if there is an exception stopping gc thread.
*/
@SuppressFBWarnings("SWL_SLEEP_WITH_LOCK_HELD")
public synchronized void shutdown() throws InterruptedException {
if (!this.running) {
return;
@@ -35,6 +35,7 @@
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.RateLimiter;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufAllocator;

@@ -576,6 +577,7 @@ public LedgerCache.LedgerIndexMetadata readLedgerIndexMetadata(long ledgerId) th
}

@Override
@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE")
public List<DetectedInconsistency> localConsistencyCheck(Optional<RateLimiter> rateLimiter) throws IOException {
long checkStart = MathUtils.nowInNano();
LOG.info("Starting localConsistencyCheck");
@@ -588,7 +588,7 @@ void shutdown() throws InterruptedException {
}
}

private class CbThreadFactory implements ThreadFactory {
private static class CbThreadFactory implements ThreadFactory {
private int counter = 0;
private String threadBaseName = "bookie-journal-callback";

@@ -48,8 +48,12 @@ public void registerStartUp() throws IOException {
for (File ledgerDir : ledgerDirsManager.getAllLedgerDirs()) {
try {
File dirtyFile = new File(ledgerDir, DIRTY_FILENAME);
dirtyFile.createNewFile();
LOG.info("Created dirty file in ledger dir: {}", ledgerDir.getAbsolutePath());
if (dirtyFile.createNewFile()) {
LOG.info("Created dirty file in ledger dir: {}", ledgerDir.getAbsolutePath());
} else {
LOG.info("Dirty file already exists in ledger dir: {}", ledgerDir.getAbsolutePath());
}

} catch (IOException e) {
LOG.error("Unable to register start-up (so an unclean shutdown cannot"
+ " be detected). Dirty file of ledger dir {} could not be created.",
@@ -24,6 +24,7 @@
import io.reactivex.rxjava3.core.Maybe;
import io.reactivex.rxjava3.core.Scheduler;
import io.reactivex.rxjava3.core.Single;
import io.reactivex.rxjava3.disposables.Disposable;
import java.io.IOException;
import java.util.Comparator;
import java.util.List;
@@ -39,7 +40,6 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;

import lombok.extern.slf4j.Slf4j;
import org.apache.bookkeeper.bookie.BookieException;
import org.apache.bookkeeper.bookie.LedgerStorage;
@@ -84,12 +84,10 @@ public DataIntegrityCheckImpl(BookieId bookieId,
}

@Override
public CompletableFuture<Void> runPreBootCheck(String reason) {
public synchronized CompletableFuture<Void> runPreBootCheck(String reason) {
// we only run this once, it could be kicked off by different checks
synchronized (this) {
if (preBootFuture == null) {
preBootFuture = runPreBootSequence(reason);
}
if (preBootFuture == null) {
preBootFuture = runPreBootSequence(reason);
}
return preBootFuture;

@@ -355,46 +353,47 @@ Throwable getThrowable() {
CompletableFuture<Set<LedgerResult>> checkAndRecoverLedgers(Map<Long, LedgerMetadata> ledgers,
String runId) {
CompletableFuture<Set<LedgerResult>> promise = new CompletableFuture<>();
Flowable.fromIterable(ledgers.entrySet())
.subscribeOn(scheduler, false)
.flatMapSingle((mapEntry) -> {
long ledgerId = mapEntry.getKey();
LedgerMetadata originalMetadata = mapEntry.getValue();
return recoverLedgerIfInLimbo(ledgerId, mapEntry.getValue(), runId)
.map(newMetadata -> LedgerResult.ok(ledgerId, newMetadata))
.onErrorReturn(t -> LedgerResult.error(ledgerId, originalMetadata, t))
.defaultIfEmpty(LedgerResult.missing(ledgerId))
.flatMap((res) -> {
try {
if (res.isOK()) {
this.ledgerStorage.clearLimboState(ledgerId);
}
return Single.just(res);
} catch (IOException ioe) {
return Single.just(LedgerResult.error(res.getLedgerId(),
res.getMetadata(), ioe));
}
});
},
true /* delayErrors */,
MAX_INFLIGHT)
.flatMapSingle((res) -> {
if (res.isOK()) {
return checkAndRecoverLedgerEntries(res.getLedgerId(),
res.getMetadata(), runId)
.map(ignore -> LedgerResult.ok(res.getLedgerId(),
res.getMetadata()))
.onErrorReturn(t -> LedgerResult.error(res.getLedgerId(),
res.getMetadata(), t));
} else {
return Single.just(res);
}
},
true /* delayErrors */,
1 /* copy 1 ledger at a time to keep entries together in entrylog */)
.collect(Collectors.toSet())
.subscribe(resolved -> promise.complete(resolved),
throwable -> promise.completeExceptionally(throwable));
final Disposable disposable = Flowable.fromIterable(ledgers.entrySet())
.subscribeOn(scheduler, false)
.flatMapSingle((mapEntry) -> {
long ledgerId = mapEntry.getKey();
LedgerMetadata originalMetadata = mapEntry.getValue();
return recoverLedgerIfInLimbo(ledgerId, mapEntry.getValue(), runId)
.map(newMetadata -> LedgerResult.ok(ledgerId, newMetadata))
.onErrorReturn(t -> LedgerResult.error(ledgerId, originalMetadata, t))
.defaultIfEmpty(LedgerResult.missing(ledgerId))
.flatMap((res) -> {
try {
if (res.isOK()) {
this.ledgerStorage.clearLimboState(ledgerId);
}
return Single.just(res);
} catch (IOException ioe) {
return Single.just(LedgerResult.error(res.getLedgerId(),
res.getMetadata(), ioe));
}
});
},
true /* delayErrors */,
MAX_INFLIGHT)
.flatMapSingle((res) -> {
if (res.isOK()) {
return checkAndRecoverLedgerEntries(res.getLedgerId(),
res.getMetadata(), runId)
.map(ignore -> LedgerResult.ok(res.getLedgerId(),
res.getMetadata()))
.onErrorReturn(t -> LedgerResult.error(res.getLedgerId(),
res.getMetadata(), t));
} else {
return Single.just(res);
}
},
true /* delayErrors */,
1 /* copy 1 ledger at a time to keep entries together in entrylog */)
.collect(Collectors.toSet())
.subscribe(resolved -> promise.complete(resolved),
throwable -> promise.completeExceptionally(throwable));
promise.whenComplete((result, ex) -> disposable.dispose());
return promise;
}

@@ -154,7 +154,15 @@ public CompletableFuture<Long> copyFromAvailable(long entryId) {
@VisibleForTesting
CompletableFuture<ByteBuf> fetchEntry(long entryId) {
List<BookieId> ensemble = metadata.getEnsembleAt(entryId);
ImmutableList<Integer> writeSet = writeSets.floorEntry(entryId).getValue().getForEntry(entryId);
final Map.Entry<Long, WriteSets> writeSetsForEntryId = this.writeSets
.floorEntry(entryId);
if (writeSetsForEntryId == null) {
log.error("writeSets for entryId {} not found, writeSets {}", entryId, writeSets);
throw new IllegalStateException("writeSets for entryId: " + entryId + " not found");
}
ImmutableList<Integer> writeSet = writeSetsForEntryId
.getValue()
.getForEntry(entryId);
int attempt = 0;
CompletableFuture<ByteBuf> promise = new CompletableFuture<>();
fetchRetryLoop(entryId, attempt,
@@ -22,6 +22,7 @@
import io.reactivex.rxjava3.core.Completable;
import io.reactivex.rxjava3.core.Flowable;
import io.reactivex.rxjava3.core.Scheduler;
import io.reactivex.rxjava3.disposables.Disposable;
import java.io.IOException;
import java.util.Iterator;
import java.util.concurrent.CompletableFuture;
@@ -76,25 +77,26 @@ Long next() throws IOException {

public CompletableFuture<Void> forEach(BiFunction<Long, LedgerMetadata, CompletableFuture<Void>> consumer) {
CompletableFuture<Void> promise = new CompletableFuture<>();
Flowable.<Long, FlatIterator>generate(
() -> new FlatIterator(ledgerManager.getLedgerRanges(zkTimeoutMs)),
(iter, emitter) -> {
try {
if (iter.hasNext()) {
emitter.onNext(iter.next());
} else {
emitter.onComplete();
}
} catch (Exception e) {
emitter.onError(e);
}
})
.subscribeOn(scheduler)
.flatMapCompletable((ledgerId) -> Completable.fromCompletionStage(processOne(ledgerId, consumer)),
false /* delayErrors */,
maxInFlight)
.subscribe(() -> promise.complete(null),
t -> promise.completeExceptionally(unwrap(t)));
final Disposable disposable = Flowable.<Long, FlatIterator>generate(
() -> new FlatIterator(ledgerManager.getLedgerRanges(zkTimeoutMs)),
(iter, emitter) -> {
try {
if (iter.hasNext()) {
emitter.onNext(iter.next());
} else {
emitter.onComplete();
}
} catch (Exception e) {
emitter.onError(e);
}
})
.subscribeOn(scheduler)
.flatMapCompletable((ledgerId) -> Completable.fromCompletionStage(processOne(ledgerId, consumer)),
false /* delayErrors */,
maxInFlight)
.subscribe(() -> promise.complete(null),
t -> promise.completeExceptionally(unwrap(t)));
promise.whenComplete((result, ex) -> disposable.dispose());
return promise;
}

@@ -22,14 +22,14 @@

import static com.google.common.base.Preconditions.checkState;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.IOException;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import java.util.Map.Entry;

import org.apache.bookkeeper.bookie.storage.ldb.KeyValueStorageFactory.DbConfigType;
import org.apache.bookkeeper.conf.ServerConfiguration;
import org.rocksdb.ColumnFamilyDescriptor;
@@ -171,6 +171,7 @@ public int get(byte[] key, byte[] value) throws IOException {
}

@Override
@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE")
public Entry<byte[], byte[]> getFloor(byte[] key) throws IOException {
try (Slice upperBound = new Slice(key);
ReadOptions option = new ReadOptions(optionCache).setIterateUpperBound(upperBound);
@@ -184,6 +185,7 @@ public Entry<byte[], byte[]> getFloor(byte[] key) throws IOException {
}

@Override
@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE")
public Entry<byte[], byte[]> getCeil(byte[] key) throws IOException {
try (RocksIterator iterator = db.newIterator(optionCache)) {
// Position the iterator on the record whose key is >= to the supplied key
@@ -235,7 +235,10 @@ public boolean clearLimbo(long ledgerId) throws IOException {
lock.lock();
try {
LedgerData ledgerData = get(ledgerId);
boolean oldValue = ledgerData != null ? ledgerData.getLimbo() : false;
if (ledgerData == null) {
throw new Bookie.NoLedgerException(ledgerId);
}
final boolean oldValue = ledgerData.getLimbo();
LedgerData newLedgerData = LedgerData.newBuilder(ledgerData).setLimbo(false).build();

if (ledgers.put(ledgerId, newLedgerData) == null) {
@@ -22,6 +22,7 @@

import com.google.common.collect.Lists;
import com.google.protobuf.ByteString;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import java.io.File;
@@ -71,6 +72,7 @@ public LedgersIndexRebuildOp(ServerConfiguration conf, boolean verbose) {
this.verbose = verbose;
}

@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE")
public boolean initiate() {
LOG.info("Starting ledger index rebuilding");

0 comments on commit 62cfc0e

Please sign in to comment.