Skip to content

Adv/code review #1680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
peter-lawrey wants to merge 9 commits into
base: develop
Choose a base branch
from
Draft

Adv/code review #1680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6abeb69
Add code-review profile tooling and smoke tests
peter-lawrey Oct 30, 2025
6464147
Update TODO targets for CQ quality follow-ups
peter-lawrey Oct 30, 2025
8f1f504
Make CLI allowed root configurable
peter-lawrey Oct 30, 2025
30664ec
Refactor documentation for clarity and consistency across multiple files
peter-lawrey Oct 30, 2025
90a1189
Broaden code-review gate enforcement
peter-lawrey Oct 30, 2025
41476e0
Enable medium SpotBugs gate with documented suppressions
peter-lawrey Oct 30, 2025
5b44d52
Harden path validation and quick SpotBugs fixes
peter-lawrey Oct 30, 2025
70edf31
Merge branch 'develop' into adv/code-review
peter-lawrey Oct 30, 2025
f4390c9
Merge branch 'develop' into adv/code-review
peter-lawrey Oct 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions TODO.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Chronicle Queue Code Review Follow-ups

- [ ] CQ-QUALITY-2170 (target: Sprint 25): Refactor StoreAppender lifecycle to remove nullable state and drop SpotBugs suppressions.
- [ ] CQ-QUALITY-2171 (target: Sprint 25): Document StoreTailer reset contract and eliminate SpotBugs nullability suppressions.
- [ ] CQ-QUALITY-2172 (target: Sprint 26): Harden SCQIndexing null handling during sequence lookup.
- [ ] CQ-QUALITY-2173 (target: Sprint 26): Introduce central CLI path validation and revisit PATH_TRAVERSAL suppressions across tooling mains.
- [ ] CQ-QUALITY-2175 (target: Sprint 27): Restore full code-review test execution/Jacoco coverage once long-running queue tests have automation-safe guards.
3 changes: 2 additions & 1 deletion docs/antora/modules/configuration/pages/timezone_rollover.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ NOTE: This feature is only relevant to daily rollovers where `RollCycle` is one
public SingleChronicleQueueBuilder rollTime(@NotNull final LocalTime rollTime, @NotNull final ZoneId zoneId);
----

NOTE: `rollTime()` allows the rollover time of the cycle file to be controlled in both Chronicle Queue open-source and Chronicle QueueEnterprise Edition. +
NOTE: `rollTime()` allows the rollover time of the cycle file to be controlled in both Chronicle Queue open-source and Chronicle QueueEnterprise Edition.
+
If this is used with in the open-source version, any `zoneId` other than `UTC` will be converted to `UTC`, and a warning message logged.

== Examples
Expand Down
178 changes: 178 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@
<additionalparam>-Xdoclint:none</additionalparam>
<sonar.organization>openhft</sonar.organization>
<sonar.host.url>https://sonarcloud.io</sonar.host.url>
<checkstyle.version>3.6.0</checkstyle.version>
<puppycrawl.version>8.45.1</puppycrawl.version>
<spotbugs.version>4.8.6</spotbugs.version>
<spotbugs.plugin.version>4.8.6.4</spotbugs.plugin.version>
<findsecbugs.version>1.14.0</findsecbugs.version>
<maven-pmd-plugin.version>3.28.0</maven-pmd-plugin.version>
<jacoco-maven-plugin.version>0.8.14</jacoco-maven-plugin.version>
<jacoco.line.coverage>0.80</jacoco.line.coverage>
<jacoco.branch.coverage>0.70</jacoco.branch.coverage>
</properties>

<dependencyManagement>
Expand Down Expand Up @@ -525,6 +534,175 @@
</build>
</profile>

<profile>
<id>code-review</id>
<activation>
<activeByDefault>false</activeByDefault>
</activation>
<properties>
<jacoco.line.coverage>0.60</jacoco.line.coverage>
<jacoco.branch.coverage>0.40</jacoco.branch.coverage>
</properties>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-checkstyle-plugin</artifactId>
<version>${checkstyle.version}</version>
<dependencies>
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>${puppycrawl.version}</version>
</dependency>
</dependencies>
<executions>
<execution>
<id>checkstyle</id>
<phase>verify</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<configLocation>src/main/config/checkstyle.xml</configLocation>
<consoleOutput>true</consoleOutput>
<failOnViolation>true</failOnViolation>
<violationSeverity>warning</violationSeverity>
<includeTestSourceDirectory>true</includeTestSourceDirectory>
<sourceDirectories>
<sourceDirectory>${project.basedir}/src/main/java</sourceDirectory>
</sourceDirectories>
<testSourceDirectories>
<testSourceDirectory>${project.basedir}/src/test/java</testSourceDirectory>
</testSourceDirectories>
</configuration>
</plugin>

<plugin>
<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs-maven-plugin</artifactId>
<version>${spotbugs.plugin.version}</version>
<dependencies>
<dependency>
<groupId>com.github.spotbugs</groupId>
<artifactId>spotbugs</artifactId>
<version>${spotbugs.version}</version>
</dependency>
<dependency>
<groupId>com.h3xstream.findsecbugs</groupId>
<artifactId>findsecbugs-plugin</artifactId>
<version>${findsecbugs.version}</version>
</dependency>
</dependencies>
<executions>
<execution>
<id>spotbugs</id>
<phase>verify</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<effort>Max</effort>
<threshold>Medium</threshold>
<failOnError>true</failOnError>
<excludeFilterFile>src/main/config/spotbugs-exclude.xml</excludeFilterFile>
<plugins>
<plugin>
<groupId>com.h3xstream.findsecbugs</groupId>
<artifactId>findsecbugs-plugin</artifactId>
<version>${findsecbugs.version}</version>
</plugin>
</plugins>
</configuration>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-pmd-plugin</artifactId>
<version>${maven-pmd-plugin.version}</version>
<executions>
<execution>
<id>pmd</id>
<phase>verify</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<failOnViolation>true</failOnViolation>
<printFailingErrors>true</printFailingErrors>
<includeTests>false</includeTests>
<rulesets>
<ruleset>src/main/config/pmd-ruleset.xml</ruleset>
</rulesets>
<excludeFromFailureFile>src/main/config/pmd-exclude.properties</excludeFromFailureFile>
</configuration>
</plugin>

<plugin>
<groupId>org.jacoco</groupId>
<artifactId>jacoco-maven-plugin</artifactId>
<version>${jacoco-maven-plugin.version}</version>
<executions>
<execution>
<id>prepare-agent</id>
<goals>
<goal>prepare-agent</goal>
</goals>
</execution>
<execution>
<id>report</id>
<phase>verify</phase>
<goals>
<goal>report</goal>
</goals>
</execution>
<execution>
<id>check</id>
<phase>verify</phase>
<goals>
<goal>check</goal>
</goals>
<configuration>
<rules>
<rule>
<element>BUNDLE</element>
<limits>
<limit>
<counter>LINE</counter>
<value>COVEREDRATIO</value>
<minimum>${jacoco.line.coverage}</minimum>
</limit>
<limit>
<counter>BRANCH</counter>
<value>COVEREDRATIO</value>
<minimum>${jacoco.branch.coverage}</minimum>
</limit>
</limits>
</rule>
</rules>
</configuration>
</execution>
</executions>
</plugin>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<reuseForks>false</reuseForks>
<trimStackTrace>false</trimStackTrace>
</configuration>
</plugin>
</plugins>
</build>
</profile>

<profile>
<id>run-benchmarks</id>
<properties />
Expand Down
6 changes: 6 additions & 0 deletions src/main/config/checkstyle-suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?xml version="1.0"?>
<!DOCTYPE suppressions PUBLIC "-//Checkstyle//DTD SuppressionFilter Experimental Configuration 1.2//EN"
"https://checkstyle.org/dtds/suppressions_1_2.dtd">
<suppressions>
<!-- Chronicle Queue: add suppressions sparingly with Jira tags and rationale. -->
</suppressions>
47 changes: 47 additions & 0 deletions src/main/config/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
<property name="charset" value="UTF-8"/>
<property name="severity" value="warning"/>

<module name="FileTabCharacter">
<property name="eachLine" value="true"/>
</module>

<module name="NewlineAtEndOfFile"/>

<module name="RegexpSingleline">
<property name="format" value="\S.*\s+$"/>
<property name="message" value="Trailing whitespace is not allowed."/>
</module>

<module name="TreeWalker">
<module name="EmptyCatchBlock">
<property name="exceptionVariableName" value="ignored|expected"/>
</module>
<module name="EmptyStatement"/>
<module name="EqualsHashCode"/>
<module name="OuterTypeFilename"/>
<module name="UnusedImports"/>
<module name="IllegalImport">
<property name="illegalPkgs" value="sun"/>
</module>
<module name="RedundantModifier">
<property name="tokens" value="CLASS_DEF,INTERFACE_DEF,ENUM_DEF,METHOD_DEF,CTOR_DEF"/>
</module>
<module name="WhitespaceAfter"/>
<module name="WhitespaceAround"/>
<module name="NoWhitespaceBefore"/>
</module>

<module name="LineLength">
<property name="max" value="240"/>
<property name="ignorePattern" value="^package\s|^import\s|aHref"/>
</module>

<module name="SuppressionFilter">
<property name="file" value="src/main/config/checkstyle-suppressions.xml"/>
<property name="optional" value="true"/>
</module>
</module>
5 changes: 5 additions & 0 deletions src/main/config/pmd-exclude.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# PMD exclusions with justifications
# Format: filepath=rule1,rule2
#
# Example:
# net/openhft/chronicle/queue/LegacyParser.java=AvoidReassigningParameters,TooManyFields
23 changes: 23 additions & 0 deletions src/main/config/pmd-ruleset.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="Chronicle Queue PMD Rules"
xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.net/ruleset_2_0_0.xsd">

<description>Broader PMD configuration for code-review profile with high-signal best practices and error-prone checks.</description>

<!-- Error-prone rules aimed at catching logic defects -->
<rule ref="category/java/errorprone.xml/EmptyCatchBlock"/>
<rule ref="category/java/codestyle.xml/EmptyControlStatement"/>
<rule ref="category/java/errorprone.xml/ReturnFromFinallyBlock"/>
<rule ref="category/java/errorprone.xml/BrokenNullCheck"/>
<rule ref="category/java/errorprone.xml/ImplicitSwitchFallThrough"/>

<!-- Best practice rules that should hold project-wide -->
<rule ref="category/java/bestpractices.xml/AvoidPrintStackTrace"/>
<rule ref="category/java/bestpractices.xml/AvoidUsingHardCodedIP"/>
<rule ref="category/java/bestpractices.xml/UnusedPrivateField"/>

<!-- Concurrency guard rails -->
<rule ref="category/java/multithreading.xml/DoubleCheckedLocking"/>
</ruleset>
Loading