Enforce and apply Google Java Style formatting (#416)

Uses [fmt-maven-plugin](https://github.com/spotify/fmt-maven-plugin) to
automatically (locally) apply [Google Java
Style](https://google.github.io/styleguide/javaguide.html) formatting
via the
[google-java-format](https://github.com/google/google-java-format)
plugin during `mvn compile` time.

During CI time, it checks for incorrect formatting on files *prior* to
compile to automatically reject failing commits so that reviewers have
fewer things to nit over and code style is more standardized.
- [x] Those file changes are initially omitted in order to validate this
part and make the PR easier to review. Will commit those once initial
review is done.

This is not *yet* enforced by a git pre-commit however, so developers
will have to add the changes that the `compile` stage applied after the
fact.

Additionally, it adds settings for
[VSCode](https://code.visualstudio.com/docs/java/java-linting#_applying-formatter-settings)
to apply these formattings during editing.
- [ ] Similar settings for Eclipse are currently missing.

Followup PR can also exclude this squashed PR from the `git blame`
analysis using a [`.git-blame-ignore-revs`
file](https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view)

Closes #406

Author: bpkroth

.github/workflows/maven.yml

@@ -50,6 +50,12 @@ jobs:
           cache: 'maven'
           distribution: 'temurin'
 
+      - name: Check formatting
+        run: mvn -B fmt:check --file pom.xml
+
+      - name: Compile with Maven
+        run: mvn -B compile test-compile --file pom.xml
+
       - name: Test with Maven
         run: mvn -B test --file pom.xml
 
@@ -605,6 +611,7 @@ jobs:
       - name: Build the benchbase docker image with all profiles using the dev image
         env:
           SKIP_TESTS: 'false'
+          DO_FORMAT_CHECKS: 'true'
         run: |
           ./docker/benchbase/build-full-image.sh
       - name: Run a basic test from the docker image against postgres test DB

.vscode/extensions.json

@@ -5,4 +5,4 @@
         "huntertran.auto-markdown-toc",
         "vscjava.vscode-java-pack"
     ]
-}
+}

.vscode/settings.json

@@ -0,0 +1,7 @@
+{
+    "files.watcherExclude": {
+        "**/target": true
+    },
+    "java.format.settings.profile": "GoogleStyle",
+    "java.format.enabled": true
+}

CONTRIBUTING.md

@@ -15,9 +15,11 @@ We welcome all contributions! Please open a [pull request](https://github.com/cm
     - [IDE](#ide)
     - [Adding a new DBMS](#adding-a-new-dbms)
     - [Java Development Notes](#java-development-notes)
-        - [Avoid var keyword](#avoid-var-keyword)
+        - [Code Style](#code-style)
         - [Compiler Warnings](#compiler-warnings)
+        - [Avoid var keyword](#avoid-var-keyword)
             - [Alternatives to arrays of generics](#alternatives-to-arrays-of-generics)
+            - [this-escape warnings](#this-escape-warnings)
 
 <!-- /TOC -->
 
@@ -31,6 +33,14 @@ Please see the existing MySQL and PostgreSQL code for an example.
 
 ## Java Development Notes
 
+### Code Style
+
+To allow reviewers to focus more on code content and not style nits, [PR #416](https://github.com/cmu-db/benchbase/pulls/416) added support for auto formatting code at compile time according to [Google Java Style](https://google.github.io/styleguide/javaguide.html) using [google-java-format](https://github.com/google/google-java-format) and [fmt-maven-plugin](https://github.com/spotify/fmt-maven-plugin) Maven plugins.
+
+Be sure to commit and include these changes in your PRs when submitting them so that the CI pipeline passes.
+
+Additionally, this formatting style is included in the VSCode settings files for this repo.
+
 ### Compiler Warnings
 
 In an effort to enforce clean, safe, maintainable code, [PR #413](https://github.com/cmu-db/benchbase/pull/413) enabled the `-Werror` and `-Xlint:all` options for the `javac` compiler.

docker/benchbase/common-env.sh

@@ -7,6 +7,8 @@ export BENCHBASE_PROFILE="${BENCHBASE_PROFILE:-postgres}"
 export CLEAN_BUILD="${CLEAN_BUILD:-true}"   # true, pre, post, false
 # Whether to run the test target during build.
 export SKIP_TESTS="${SKIP_TESTS:-false}"
+# Whether to do format checks during build (mostly for CI).
+export DO_FORMAT_CHECKS="${DO_FORMAT_CHECKS:-false}"
 
 # Setting this allows us to easily tag and publish the image name in our CI pipelines or locally.
 CONTAINER_REGISTRY_NAME="${CONTAINER_REGISTRY_NAME:-}"

docker/benchbase/devcontainer/build-in-container.sh

@@ -10,6 +10,7 @@ set -eu -o pipefail
 BENCHBASE_PROFILES="${BENCHBASE_PROFILES:-cockroachdb mariadb mysql oracle phoenix postgres spanner sqlite sqlserver}"
 CLEAN_BUILD="${CLEAN_BUILD:-true}"    # true, false, pre, post
 SKIP_TESTS="${SKIP_TESTS:-false}"
+DO_FORMAT_CHECKS="${DO_FORMAT_CHECKS:-false}"
 
 cd /benchbase
 mkdir -p results
@@ -95,6 +96,12 @@ else
     echo "WARNING: Unhandled SKIP_TESTS mode: '$SKIP_TESTS'" >&2
 fi
 
+# Pre format checks are mostly for CI.
+# Else, things are reformatted during compile.
+if [ "${DO_FORMAT_CHECKS:-}" == 'true' ]; then
+    mvn -B --file pom.xml fmt:check
+fi
+
 # Fetch resources serially to work around mvn races with downloading the same
 # file in multiple processes (mvn uses *.part instead of use tmpfile naming).
 for profile in ${BENCHBASE_PROFILES}; do

docker/benchbase/run-dev-image.sh

@@ -36,6 +36,7 @@ docker run ${INTERACTIVE_ARGS:-} --rm \
     --env BENCHBASE_PROFILES="$BENCHBASE_PROFILES" \
     --env CLEAN_BUILD="$CLEAN_BUILD" \
     --env SKIP_TESTS="$SKIP_TESTS" \
+    --env DO_FORMAT_CHECKS="$DO_FORMAT_CHECKS" \
     --env EXTRA_MAVEN_ARGS="${EXTRA_MAVEN_ARGS:-}" \
     --user "$CONTAINERUSER_UID:$CONTAINERUSER_GID" \
     -v "$MAVEN_CONFIG:/home/containeruser/.m2" \

pom.xml

@@ -323,12 +323,25 @@
             <artifactId>janino</artifactId>
             <version>3.1.11</version>
         </dependency>
-
     </dependencies>
 
     <build>
         <directory>${buildDirectory}</directory>
         <plugins>
+            <plugin>
+                <!-- format/check code via google style format -->
+                <groupId>com.spotify.fmt</groupId>
+                <artifactId>fmt-maven-plugin</artifactId>
+                <version>2.21.1</version>
+                <executions>
+                    <execution>
+                        <goals>
+                            <goal>check</goal>
+                            <goal>format</goal>
+                        </goals>
+                    </execution>
+                </executions>
+            </plugin>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-compiler-plugin</artifactId>

src/main/java/com/oltpbenchmark/BenchmarkState.java

@@ -18,109 +18,99 @@
 package com.oltpbenchmark;
 
 import com.oltpbenchmark.types.State;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicInteger;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public final class BenchmarkState {
 
-    private static final Logger LOG = LoggerFactory.getLogger(BenchmarkState.class);
+  private static final Logger LOG = LoggerFactory.getLogger(BenchmarkState.class);
 
-    private final long testStartNs;
-    private final CountDownLatch startBarrier;
-    private final AtomicInteger notDoneCount;
-    private volatile State state = State.WARMUP;
+  private final long testStartNs;
+  private final CountDownLatch startBarrier;
+  private final AtomicInteger notDoneCount;
+  private volatile State state = State.WARMUP;
 
-    /**
-     * @param numThreads number of threads involved in the test: including the
-     *                   master thread.
-     */
-    public BenchmarkState(int numThreads) {
-        startBarrier = new CountDownLatch(numThreads);
-        notDoneCount = new AtomicInteger(numThreads);
+  /**
+   * @param numThreads number of threads involved in the test: including the master thread.
+   */
+  public BenchmarkState(int numThreads) {
+    startBarrier = new CountDownLatch(numThreads);
+    notDoneCount = new AtomicInteger(numThreads);
 
+    testStartNs = System.nanoTime();
+  }
 
-        testStartNs = System.nanoTime();
-    }
+  // Protected by this
 
-    // Protected by this
+  public long getTestStartNs() {
+    return testStartNs;
+  }
 
-    public long getTestStartNs() {
-        return testStartNs;
+  public State getState() {
+    synchronized (this) {
+      return state;
     }
+  }
 
-    public State getState() {
-        synchronized (this) {
-            return state;
-        }
+  /** Wait for all threads to call this. Returns once all the threads have entered. */
+  public void blockForStart() {
+
+    startBarrier.countDown();
+    try {
+      startBarrier.await();
+    } catch (InterruptedException e) {
+      throw new RuntimeException(e);
     }
+  }
 
-    /**
-     * Wait for all threads to call this. Returns once all the threads have
-     * entered.
-     */
-    public void blockForStart() {
+  public void startMeasure() {
+    state = State.MEASURE;
+  }
 
+  public void startColdQuery() {
+    state = State.COLD_QUERY;
+  }
 
-        startBarrier.countDown();
-        try {
-            startBarrier.await();
-        } catch (InterruptedException e) {
-            throw new RuntimeException(e);
-        }
-    }
+  public void startHotQuery() {
+    state = State.MEASURE;
+  }
 
-    public void startMeasure() {
-        state = State.MEASURE;
-    }
+  public void signalLatencyComplete() {
+    state = State.LATENCY_COMPLETE;
+  }
 
-    public void startColdQuery() {
-        state = State.COLD_QUERY;
-    }
+  public void ackLatencyComplete() {
+    state = State.MEASURE;
+  }
 
-    public void startHotQuery() {
-        state = State.MEASURE;
-    }
+  public void signalError() {
+    // A thread died, decrement the count and set error state
+    notDoneCount.decrementAndGet();
+    state = State.ERROR;
+  }
 
-    public void signalLatencyComplete() {
-        state = State.LATENCY_COMPLETE;
-    }
+  public void startCoolDown() {
+    state = State.DONE;
 
-    public void ackLatencyComplete() {
-        state = State.MEASURE;
-    }
+    // The master thread must also signal that it is done
+    signalDone();
+  }
 
-    public void signalError() {
-        // A thread died, decrement the count and set error state
-        notDoneCount.decrementAndGet();
-        state = State.ERROR;
-    }
+  /** Notify that this thread has entered the done state. */
+  public int signalDone() {
 
-    public void startCoolDown() {
-        state = State.DONE;
+    int current = notDoneCount.decrementAndGet();
 
-        // The master thread must also signal that it is done
-        signalDone();
+    if (LOG.isDebugEnabled()) {
+      LOG.debug(String.format("%d workers are not done. Waiting until they finish", current));
     }
-
-    /**
-     * Notify that this thread has entered the done state.
-     */
-    public int signalDone() {
-
-        int current = notDoneCount.decrementAndGet();
-
-        if (LOG.isDebugEnabled()) {
-            LOG.debug(String.format("%d workers are not done. Waiting until they finish", current));
-        }
-        if (current == 0) {
-            // We are the last thread to notice that we are done: wake any
-            // blocked workers
-            this.state = State.EXIT;
-        }
-        return current;
+    if (current == 0) {
+      // We are the last thread to notice that we are done: wake any
+      // blocked workers
+      this.state = State.EXIT;
     }
-
-}
+    return current;
+  }
+}