Skip to content

OAK-12239: Remove FT_PREFETCH_OAK-9780 and remove 'no prefetch' code path#2934

Open
reschke wants to merge 5 commits into
trunkfrom
OAK-12239
Open

OAK-12239: Remove FT_PREFETCH_OAK-9780 and remove 'no prefetch' code path#2934
reschke wants to merge 5 commits into
trunkfrom
OAK-12239

Conversation

@reschke

@reschke reschke commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

No description provided.

}

@Nullable
public Feature getPrefetchFeature() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

setPrefetchFeature() was removed but getPrefetchFeature() and the prefetchFeature field remain (always null). Consider removing the getter and dead fields on the builder, store, and service as part of this cleanup.

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.

indeed

System.setProperty(SYS_PROP_PREFETCH, "true");
}

@AfterClass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After dropping sys-prop setup, consider adding a test that calls DocumentNodeStore.prefetch() (not CacheWarming directly) with default builder setup and asserts cache warming — that would lock in the always-on store path this PR introduces.

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.

I don't quite understand; is this about adding a test that we should have had before?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rishabhdaim

rishabhdaim commented Jun 5, 2026

Copy link
Copy Markdown
Contributor
  1. oak-store-document/AGENTS.md still lists FT_PREFETCH_OAK-9780 and setPrefetchFeature() — both removed here.
  2. Store-level prefetch is always on when prefetch() is called; PrefetchDispatcher is unrelated and still gated by prefetchExternalChanges.
  3. Please update the Prefetch section and drop that row from the toggles table.

@reschke reschke requested a review from rishabhdaim June 5, 2026 13:53
@reschke

reschke commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

@rishabhdaim - thx for the review, and please see my questions (PR minimally updated).

(this shows that even removing a toggle is not totally trivial)

@rishabhdaim

Copy link
Copy Markdown
Contributor

Outside this diff — CacheWarmingTest.prefetch() (lines ~138–169):

prefetch() still exercises CacheWarming directly, so it won't catch a regression in DocumentNodeStore.prefetch() now that prefetch is always-on at the store layer. Call the store API instead (same assertions):

-        CacheWarming cw = new CacheWarming(ds);
         DocumentNodeStore store = builderProvider.newBuilder().setAsyncDelay(0)
                 .setDocumentStore(ds).getNodeStore();
         ...
-        cw.prefetch(children, store.getRoot());
+        store.prefetch(children, store.getRoot());
         ...
-        cw.prefetch(paths, store.getRoot());
+        store.prefetch(paths, store.getRoot());

@sonarqubecloud

sonarqubecloud Bot commented Jun 5, 2026

Copy link
Copy Markdown

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.

2 participants