[#119] Migrate connector-server-jetty off deprecated org.eclipse.jetty.util.log

[vharseko]

Fixes #119. Follow-up to #115.

Problem

OpenICFWebSocketCreator and OpenICFWebSocketServletBase were the last two classes in connector-server-jetty still using org.eclipse.jetty.util.log.Log/Logger — deprecated in Jetty 10/11 (a shim over SLF4J) and removed in Jetty 12, so they block a future Jetty 12 upgrade. The OpenICFWebSocket endpoint added in #115 already uses the framework's own org.identityconnectors.common.logging.Log.

Change

• Both classes now use org.identityconnectors.common.logging.Log (Log.getLog(...)), matching OpenICFWebSocket.
• The framework Log has no bare warn(Throwable) overload, so the three catch blocks in OpenICFWebSocketServletBase.destroy() gain messages: "Failed to close the WebSocket creator" / "Failed to shut the executor service down" / "Failed to release the connector framework".
• The framework Log formats through MessageFormat, which treats single quotes as escape characters — the quotes in Creating single 'anonymous' authenticator are doubled (''anonymous'') so the message survives intact.
• Drive-by: removed the unused ByteBuffer and WebSocketPingPongListener imports in OpenICFWebSocketServletBase.

No pom changes (org.eclipse.jetty.util.StringUtil is not deprecated and stays). After this change the module has no references to org.eclipse.jetty.util.log left.

Note for deployments: these few messages move from the Jetty/SLF4J route to the OpenICF LogSpi (org.identityconnectors.common.logging.class), consistent with the rest of the framework.

Tests

Pure logging swap — no behavior change, no new tests. Existing suite covers the touched lines (creator constructor and lazy configure() path). Verified: connector-server-jetty 34/34.

[maximthomas]

The detection half is solid — countMarkedVersions terminates safely (getLength() masks to 0x7FFF, so from advances by at least LENGTH_PER_VERSION), the dedicated scan is the right call over extending VersionVisitor, and MVVTest (40/40) + IntegrityCheckTest (10/10) are green on the branch.

The problem is the remediation the PR prescribes. Requesting changes on that.

icheck -p remediation silently resurrects deleted keys (high)

The PR says "icheck -p remains the remediation path … marks are reported first and then rewritten by the prune." That is not safe: MVV.prune uses the same bit as its private "keep this version" flag and never clears pre-existing marks before pass 1. A leftover mark is therefore read as prune's own decision to keep:

• persistit/core/src/main/java/com/persistit/MVV.java:530-546 — the primordial branch promotes the first marked version it finds. With a stale mark on an older version, the obsolete version wins. Note the branch is still entered: marked counts only prune's own mark() calls (1), while two versions actually carry the bit.
• persistit/core/src/main/java/com/persistit/MVV.java:508-524 — pass 2 registers only !isMarked versions in prunedVersionList. The marked version that pass 3 doesn't promote is therefore discarded without ever being registered, so its long-record page (lines 515-520) is never deallocated. Observed: prunedVersionList is empty in the stale-mark run vs. one entry in the control.

Reproduced on this branch at unit level (two committed versions, mark on the older one, MVV.prune called directly):

leftoverMark=false  survivor=[22 22]     prunedVersionList.size=1   <- correct, newest version
leftoverMark=true   survivor=[11 11 11]  prunedVersionList.size=0   <- stale version promoted

And end-to-end through the exact path the PR recommends — testPruneClearsLeftoverMarkBits plus a before/after snapshot of the visible key set, with a control run that calls corrupt5's ex.store() but skips MVV.mark:

>>> doMark=false  markedVersions=0   faults=0   keysBefore=500  keysAfter=500
>>> doMark=true   markedVersions=10  faults=10  keysBefore=500  keysAfter=505

icheck -p undeleted 5 transactionally removed keys. The mark sat on the value version, so prune promoted it instead of the AntiValue. The control rules out corrupt5's non-transactional store as the cause.

The bug lives in MVV.prune and predates this PR, but this PR is what tells operators to run icheck -p on affected volumes, and testPruneClearsLeftoverMarkBits asserts remediation "works" while only checking counters. Suggested:

1. Make prune defensive about its own invariant — clear any pre-existing marks before pass 1 (you already have the scan):

// MVV.prune, before the first pass
if (countMarkedVersions(bytes, offset, length) > 0) {
    int from = offset + 1;
    while (from + LENGTH_PER_VERSION <= offset + length) {
        final int vlength = getLength(bytes, from);
        unmark(bytes, from);
        from += vlength + LENGTH_PER_VERSION;
    }
}

2. Add the data-preservation assertion to testPruneClearsLeftoverMarkBits in
   persistit/core/src/test/java/com/persistit/IntegrityCheckTest.java:132 — snapshot the visible key/value set before corruption, compare after remediation. That single assertion is what surfaces this.

icheck -P (pruneAndClear) regresses on affected volumes (medium)

_faults is never cleared (reset() at persistit/core/src/main/java/com/persistit/IntegrityCheck.java:786 doesn't touch it), and the new fault is added during buffer.verify — i.e. before the prune in the same verifyPage call. So the gate at persistit/core/src/main/java/com/persistit/IntegrityCheck.java:315 can never pass on an affected volume: resetMVVCounts is skipped even though the prune fully rewrote the marks.

Confirmed by running the real runTask() path (icheck trees=* -u -P -v), against a control that stores the same records without setting the mark bit:

>>> doMark=false  marked=0   faults=0   -> "0 aborted transactions were cleared by pruning"
>>> doMark=true   marked=10  faults=10  -> "PruneAndClear failed to remove all aborted MMVs"

Before this PR the same volume ran -P to completion.

Options: keep marked-version faults in a separate list excluded from the -P gate; re-verify after pruning and drop the fault when the count reaches zero; or at minimum document "run icheck -P twice".

Marked-version count runs on an unvalidated MVV (low)

persistit/core/src/main/java/com/persistit/IntegrityCheck.java:206 calls countMarkedVersions before visitAllVersions validates the record, so a structurally corrupt MVV is walked with garbage lengths. Confirmed — an MVV with only a damaged length field and no mark bit anywhere:

intact  -> marked=0
corrupt -> marked=1

That yields a bogus MVV has 1 version with a leftover mark bit fault and an inflated MarkedVersions counter next to the genuine CorruptValueException fault — misleading exactly when an operator is using the counter to answer "am I affected by #286?". The inline comment says the count "must be counted before visitAllVersions", but there is no such constraint — visitAllVersions does not mutate bytes. Counting after a clean traversal (or sharing one traversal) fixes it.

Nits

• Doc drift: persistit/doc/Management.rst:136 documents the icheck -c CSV header verbatim and isn't updated with MarkedVersions. Worth noting in the PR description that both the CSV columns and the plain-report format changed — breaking for anything parsing icheck output.
• Redundant page tracking: verifyPage already holds page at persistit/core/src/main/java/com/persistit/IntegrityCheck.java:1187; a field set there would remove the visitPage override and the duplicated _currentPage state.
• Copyright header: MVV.java and MVVTest.java carry Portions Copyrighted 2026 3A Systems, LLC; persistit/core/src/test/java/com/persistit/IntegrityCheckTest.java is modified without it (recent commits 3f8c98909, cca1422d7 do add it).
• corrupt5 duplication: it is a byte-for-byte copy of corrupt2 except for the mutation — a shared helper taking the mutation would be cleaner. Both also leak ignoreMVCCFetch(true) if the loop throws (no finally).
• Test granularity: testLeftoverMarkBitsReported marks one version per record, so faults == markedVersionCount by construction. Marking two versions in one record would cover the counter/fault distinction and the plural() branch.
• Line length: persistit/core/src/main/java/com/persistit/IntegrityCheck.java:209 is 121 chars, one over the file's 120-col wrap (cosmetic — no formatter config in the repo).

[vharseko]

@maximthomas Thanks for the detailed review, but it looks like it landed on the wrong PR.

This PR only migrates connector-server-jetty off the deprecated org.eclipse.jetty.util.log API — two files under OpenICF-java-framework/connector-server-jetty, no other changes.

All of the findings reference Persistit code — persistit/core/src/main/java/com/persistit/MVV.java, IntegrityCheck.java, the icheck -p/-P remediation, Management.rst — none of which exist in OpenICF. That codebase lives in the OpenDJ repository (the forgerock-persistit module), so the review presumably belongs to a Persistit "leftover mark bits" PR there.

Could you re-post the review on the intended PR and dismiss it here? The CHANGES_REQUESTED status is currently blocking the merge of this unrelated change.

[maximthomas]

Sorry, really wrong PR. This PR is LGTM