Skip to content

OAK-8840: copy jackrabbit-data into oak-blob#2810

Open
reschke wants to merge 24 commits intotrunkfrom
OAK-8840
Open

OAK-8840: copy jackrabbit-data into oak-blob#2810
reschke wants to merge 24 commits intotrunkfrom
OAK-8840

Conversation

@reschke
Copy link
Copy Markdown
Contributor

@reschke reschke commented Mar 21, 2026

This change change affects lots of classes, due to refactoring. Inspecting individual commits should be easier. Will explain the different aspects later.

(I believe we should look at IntelliJ/Sonar warnings in a separate step)

--->>> https://issues.apache.org/jira/browse/OAK-8840?focusedCommentId=18068214&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-18068214

@reschke reschke changed the title OAK-8840: copy jackrabbit-data into Oak: copy java code (not tests) OAK-8840: copy jackrabbit-data into Oak Mar 21, 2026
@reschke reschke marked this pull request as draft March 21, 2026 10:37
@reschke reschke changed the title OAK-8840: copy jackrabbit-data into Oak OAK-8840: copy jackrabbit-data into oak-blob Mar 22, 2026
@reschke reschke marked this pull request as ready for review March 25, 2026 08:43
@reschke reschke self-assigned this Mar 25, 2026
@reschke
Copy link
Copy Markdown
Contributor Author

reschke commented Mar 30, 2026

Note that bad test coverage of course is not caused by this PR.

@reschke reschke marked this pull request as draft March 31, 2026 04:46
@reschke reschke marked this pull request as ready for review March 31, 2026 04:52
Comment on lines +166 to +173
if (pos > 0) {
long skipped = is.skip(pos);
if (skipped < pos) {
return -1;
}
}

return is.read(buff, off, length);
Copy link
Copy Markdown
Member

@thomasmueller thomasmueller Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to use a loop to skip correctly. Something like this:

Suggested change
if (pos > 0) {
long skipped = is.skip(pos);
if (skipped < pos) {
return -1;
}
}
return is.read(buff, off, length);
long remaining = pos;
while (remaining > 0) {
long skipped = is.skip(remaining);
if (skipped <= 0) {
return -1;
}
remaining -= skipped;
}

Or use is.skipNBytes(pos) (Java 12+) which throws on short skip, or IOUtils.skipFully() from Commons IO.

And then we should have a test case for this.

Copy link
Copy Markdown
Contributor Author

@reschke reschke Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then we should have a test case for this.

I'll have a look.

Copy link
Copy Markdown
Contributor Author

@reschke reschke Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomasmueller - the code that instantiates the wrapper has indeed test coverage (Jcr2ToSegmentTest, maybe more), but none of the methods are actually invoked.

Optimally, we would have a test that validates before/after behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, writing a test that exercises just the skipping method is simple, but it would only prove that skipNBytes works, which we should assume.

The only other thing that actually would be interesting would be a real IT.

Given the fact that this is "almost dead" code anyway, I'd prefer not to do it.

Instead, we should remove the "upgrade" code path (JR->OAK) and just leave the "sidegrade" functionality in. Should I open a separate ticket for that? (cc @mbaedke )

reschke and others added 4 commits March 31, 2026 14:59
…2811)

* OAK-12145 : add compatibility tests for Caffeine migration (PR 2807)

Add implementation-independent test coverage for all classes affected
by the Guava-to-Caffeine cache migration in OAK-11946. Tests reference
only Oak-level types (CacheLIRS, CacheStats, DiffCache, etc.) so the
same suite can be cherry-picked to OAK-11946 and run unchanged; any
failure there is a migration compatibility gap.

Modules covered:
- oak-core-spi: AbstractCacheStats, CacheLIRS, EmpiricalWeigher
- oak-store-document: DocumentNodeStoreBuilder, NodeDocumentCache,
  MemoryDiffCache, LocalDiffCache, TieredDiffCache,
  CachingCommitValueResolver, DocumentNodeStore, PersistentCache
- oak-run-commons: DocumentNodeStoreHelper
- oak-search: ExtractedTextCache (stats tracking)
- oak-search-elastic: ElasticIndexStatistics (cache, refresh, failure)
- oak-segment-tar: SegmentCache (loader failure contract)
- oak-blob: BlobIdSet (cache miss / persistence semantics)
- oak-blob-cloud: S3Backend (expiry, cache enable/disable)
- oak-blob-cloud-azure: AzureBlobStoreBackend,
  AzureBlobStoreBackendV8 (expiry, cache enable/disable)

* OAK-12145: Clarify cache compatibility test intent

* OAK-12145: Rename S3 compatibility test

* OAK-12145: Make cache compatibility tests behavior-based

* OAK-12145: Fix SegmentCache compatibility assertion
@reschke reschke requested a review from thomasmueller April 1, 2026 12:34
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 1, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
43.7% Coverage on New Code (required ≥ 80%)
E Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants