Skip to content

Commit 806ee61

Browse files
authored
Improve instructions for jdk8 branch update (#1298)
## Summary TLDR : update jdk8 instructions to include more details (so as to take lesser time in jdk 8 branch update) More detailed changes : - Spotless: update from skip-profile approach to remove entirely (no plugin, no pluginManagement, no version property) - Add OWASP `dependency-check-maven` plugin removal step - Add `coverageReport.yml` transformation (JDK 8, 80% threshold, remove JVM17+ group filter) - Add Java 9+ API replacement table (`List.of`, `Optional.isEmpty`, `LocalDate.EPOCH`, `URLDecoder.decode(Charset)`, etc.) - Add JDBC 4.3 test method removals (`enquoteLiteral`, `enquoteIdentifier`) - Add `IntervalConverter` `Duration.ofNanos(Long.MIN_VALUE)` test removal with explanation - Add `DatabricksDriverFeatureFlagsContext` hang fix guidance for JDK 8 - Add rule: only modify `coverageReport.yml`, keep all other workflow files as-is - Preserve existing TODOs with added context comments NO_CHANGELOG=true --------- Signed-off-by: Samikshya Chand <samikshya.chand@databricks.com> Signed-off-by: samikshya-chand_data <samikshya.chand@databricks.com>
1 parent 93f01a2 commit 806ee61

1 file changed

Lines changed: 65 additions & 16 deletions

File tree

.claude/commands/sync-jdk8-branch.md

Lines changed: 65 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,9 @@ Read each file before editing. Apply all of the following.
4242
#### `pom.xml` (root)
4343

4444
- Set `maven.compiler.source` and `maven.compiler.target` to `1.8`
45-
- Ensure the `jdk8` spotless-skip profile exists (add if missing):
46-
```xml
47-
<profile>
48-
<!-- Skip spotless on JDK 8: spotless-maven-plugin 2.39.0 requires Java 11+ -->
49-
<id>jdk8</id>
50-
<activation><jdk>1.8</jdk></activation>
51-
<properties><spotless.skip>true</spotless.skip></properties>
52-
</profile>
53-
```
54-
- TODO: Investigate whether an older version of `spotless-maven-plugin` (pre-2.28.0) supports JDK 8 so formatting can be enforced instead of skipped.
45+
- **Remove spotless entirely**: delete the `spotless-maven-plugin` plugin declaration, any `<pluginManagement>` entry for it, and any `spotless.version` property. Do NOT add a skip profile — the plugin must not exist on the jdk-8 branch at all.
46+
<!-- Spotless is removed entirely (not just skipped) because even loading spotless-maven-plugin 2.39.0 on JDK 8 causes CI failures regardless of skip flags. TODO: Investigate whether an older version (pre-2.28.0) supports JDK 8 so formatting can be enforced rather than absent. -->
47+
- **Remove `org.owasp:dependency-check-maven`** plugin entirely — remove from both `<plugins>` and any `<pluginManagement>` entry. Not needed on the jdk-8 branch.
5548
- Pin these dependency versions:
5649

5750
| Property | jdk-8 value | Reason |
@@ -79,9 +72,51 @@ Read each file before editing. Apply all of the following.
7972
<exclude>**/integration/e2e/**/*.java</exclude>
8073
```
8174

82-
#### Source code
75+
#### `.github/workflows/coverageReport.yml`
76+
77+
Change the JDK version to 8 and lower the coverage threshold to 80%:
78+
79+
```yaml
80+
- name: Set up JDK
81+
uses: actions/setup-java@v4
82+
with:
83+
java-version: '8'
84+
distribution: 'adopt'
85+
```
86+
87+
Update every occurrence of `85` in the threshold check to `80`. Also remove `-Dgroups='!Jvm17PlusAndArrowToNioReflectionDisabled'` from the test run command (that tag doesn't exist on the jdk-8 branch) and add `-Dspotless.skip=true`:
88+
89+
```bash
90+
mvn -pl jdbc-core clean test -Dspotless.skip=true jacoco:report
91+
```
92+
93+
**Do not modify any other workflow files.** All other `.github/workflows/*.yml` files must be left exactly as merged from main.
94+
95+
#### Source code — Java 9+ API replacements
8396

84-
- **Remove JDBC 4.3 methods** from `DatabricksConnection.java``setShardingKeyIfValid` and `setShardingKey` overloads plus the `ShardingKey` import. These require Java 9+. Check for any other JDBC 4.3 APIs (`ConnectionBuilder`, `PooledConnectionBuilder`) and remove those too.
97+
Scan all Java source files for APIs introduced after JDK 8 and replace them. Common patterns:
98+
99+
| Java 9+ (incompatible) | JDK 8 replacement |
100+
|---|---|
101+
| `List.of(...)` | `Arrays.asList(...)` (or `Collections.unmodifiableList(Arrays.asList(...))`) |
102+
| `Set.of(...)` | `ImmutableSet.of(...)` (Guava) |
103+
| `Map.of(...)` | `ImmutableMap.of(...)` (Guava) |
104+
| `Map.entry(k, v)` | `new AbstractMap.SimpleEntry<>(k, v)` (or Guava's `Maps.immutableEntry(k, v)`) |
105+
| `Optional.isEmpty()` | `!optional.isPresent()` |
106+
| `String.repeat(n)` | loop or `new String(new char[n]).replace('\0', c)` |
107+
| `InputStream.nullInputStream()` | `new ByteArrayInputStream(new byte[0])` |
108+
| `Reader.nullReader()` | `new StringReader("")` |
109+
| `LocalDate.EPOCH` | `LocalDate.of(1970, 1, 1)` |
110+
| `URLDecoder.decode(str, StandardCharsets.UTF_8)` (Charset overload, Java 10+) | `URLDecoder.decode(str, StandardCharsets.UTF_8.name())` |
111+
112+
After replacing, verify no stragglers remain:
113+
```bash
114+
grep -r "List\.of\|Set\.of\|Map\.of\|Map\.entry\|Optional\.isEmpty\|\.repeat(\|nullInputStream\|nullReader\|LocalDate\.EPOCH" src/main/java/ src/test/java/
115+
```
116+
117+
#### Source code — Other changes
118+
119+
- **Remove JDBC 4.3 methods** from `DatabricksConnection.java` — `setShardingKeyIfValid` and `setShardingKey` overloads plus the `ShardingKey` import. Also remove JDBC 4.3 test methods such as `enquoteLiteral`, `enquoteIdentifier`, and any other JDBC 4.3+ APIs (`ConnectionBuilder`, `PooledConnectionBuilder`). All require Java 9+.
85120

86121
- **Remove Arrow patch files** (introduced in PR #1243 for JDK 16+ NIO restrictions — not relevant to JDK 8):
87122

@@ -109,6 +144,17 @@ Read each file before editing. Apply all of the following.
109144
- `src/test/resources/arrow/` (entire directory)
110145
- Remove any test annotated with `@Tag("Jvm17PlusAndArrowToNioReflectionDisabled")`
111146

147+
- **Remove `IntervalConverter` overflow test**: Delete the test case for `Duration.ofNanos(Long.MIN_VALUE)` in `IntervalConverterTest` (or wherever it lives). On JDK 8, `Duration.toNanos()` uses `Math.multiplyExact()` internally and throws `ArithmeticException` when seconds × 1,000,000,000 overflows `long`. JDK 17+ doesn't have this problem. The edge case doesn't occur in practice — remove the test rather than working around it.
148+
149+
- **Fix `DatabricksDriverFeatureFlagsContext` hanging test**: Any test that constructs `DatabricksDriverFeatureFlagsContext` with a fake hostname (e.g. `sample-host.cloud.databricks.com`) will hang indefinitely on JDK 8. JDK 8's HTTP client has no default connection timeout, so the TCP attempt blocks for the full OS socket timeout (minutes+). JDK 17+ fails fast because of tighter defaults.
150+
151+
Fix in order of preference:
152+
1. Mock the HTTP client / feature-flags fetcher so no real network call is made.
153+
2. Pass a pre-populated flag map to the constructor instead of fetching from the server.
154+
3. Set an explicit connection timeout before the test runs.
155+
156+
Never leave a test that makes a real outbound HTTP/TCP connection to a non-existent host on the jdk-8 branch — it will cause the CI job to time out.
157+
112158
#### Full dependency audit
113159

114160
Check **every** dependency across all `pom.xml` files for JDK 8 compatibility. A version bump from `main` can silently raise the minimum Java version.
@@ -121,7 +167,8 @@ Known incompatible versions:
121167
| `org.mockito:mockito-*` | 4.x |
122168
| `com.nimbusds:nimbus-jose-jwt` | 9.x |
123169
| `org.wiremock:wiremock` | remove entirely |
124-
| `com.diffplug.spotless:spotless-maven-plugin` | skip via profile |
170+
| `com.diffplug.spotless:spotless-maven-plugin` | remove entirely (not just skip) |
171+
| `org.owasp:dependency-check-maven` | remove entirely |
125172
| `jakarta.annotation:jakarta.annotation-api` | 1.x (2.x requires Java 11) |
126173

127174
Everything else in the current `pom.xml` is JDK 8 compatible. For any new dependency not in this table: check Maven Central / release notes. If no JDK 8 compatible version exists, **stop and notify the user** with the options (pin older version, swap alternative, remove) before taking action.
@@ -171,7 +218,9 @@ Share the PR URL with the user.
171218

172219
- Never use `--add-opens`, `--add-exports`, or any JPMS flag — invalid on JDK 8.
173220
- Never add branching for JDK 9, 11, 17, or 21 — the branch targets JDK 8 only.
174-
- Never run `mvn spotless:apply` on JDK 8the `jdk8` profile skips it automatically.
175-
- TODO: Investigate whether an older `spotless-maven-plugin` version (pre-2.28.0) supports JDK 8 so formatting can be enforced rather than skipped.
176-
- Never add JDBC 4.3+ APIs (`ShardingKey`, `ConnectionBuilder`, `PooledConnectionBuilder`).
221+
- Never run `mvn spotless:apply` on JDK 8. Spotless is fully removed from the jdk-8 branch (no plugin declaration, no pluginManagement entry, no version property) because even loading the plugin on JDK 8 causes CI failures. Always pass `-Dspotless.skip=true` as a safety net.
222+
<!-- TODO: Investigate whether an older `spotless-maven-plugin` version (pre-2.28.0) supports JDK 8 so formatting can be enforced rather than absent. -->
223+
- Never add JDBC 4.3+ APIs (`ShardingKey`, `ConnectionBuilder`, `PooledConnectionBuilder`, `enquoteLiteral`, `enquoteIdentifier`).
177224
- Only unit tests belong in the jdk-8 branch. Never carry over fakeservice or e2e tests — fakeservice depends on WireMock 3.x (Java 11+), and e2e tests are covered by comprehensive MultiDBR testing. Remove them and their test resources entirely.
225+
- Never leave a test that makes a real outbound HTTP/TCP connection to a non-existent host — JDK 8 will hang indefinitely without a connection timeout.
226+
- Only modify `coverageReport.yml` in `.github/workflows/`. All other workflow files must be left exactly as merged from main.

0 commit comments

Comments
 (0)