SOLR-16286 : Topic stream not honoring initialCheckpoint in getPersis…

[danrosher]

https://issues.apache.org/jira/browse/SOLR-16286

Description

getCheckpoints() does honor initialCheckpoint, but when stored, getPersistedCheckpoints which is processed first, does not. The effect is that initialCheckpoint=0 doesn't work as expected after it's stored.

Solution

Modify getPersistedCheckpoints to honor initialCheckpoint as it does in getCheckpoints

Tests

Added StreamExpressionTest.testTopicStreamInitialCheckpoint to do run a topic SE, then persist it, then re-run to ensure initialCheckpoint=0 is still honored, then again without initialCheckpoint=0 to ensure zero returned as up to date.,

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• I have created a Jira issue and added the issue ID to my pull request title.
• I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
• I have developed this patch against the main branch.
• I have run ./gradlew check.
• I have added tests for my changes.
• I have added documentation for the Reference Guide

[danrosher]

./gradlew check passed except:

`
Crawl/parse...

Verify...

.../solr/solr/documentation/build/site/modules/jwt-auth/org/apache/solr/handler/admin/api/ModifyJWTAuthPluginConfigAPI.html
BROKEN LINK: .../solr/solr/documentation/build/site/core/org/apache/solr/handler/admin/api/JWTConfigurationPayload.html
BROKEN LINK: .../solr/solr/documentation/build/site/core/org/apache/solr/handler/admin/api/JWTConfigurationPayload.html

Broken javadocs links were found! Common root causes:

• A typo of some sort for manually created links.
• Public methods referencing non-public classes in their signature.
  ~
  `
  unsure why this failed though

[cpoerschke]

Thanks @danrosher for opening this PR!

[cpoerschke]

Broken javadocs links were found! Common root causes:

• A typo of some sort for manually created links.
• Public methods referencing non-public classes in their signature.
  ~
  `
  unsure why this failed though

#944 sounds like it might be related.

[cpoerschke]

So this is the same expression as above but without the initialCheckpoint=0 ... though reading the (current) docs that means "the highest version in the index" though if the highest version in the index was used then the first batch in the stream below would not include the documents just added with ids 2 and 3?

Wondering if the documentation needs tweaking to account for persisted checkpoints?

https://github.com/apache/solr/blob/releases/solr/9.0.0/solr/solr-ref-guide/modules/query-guide/pages/stream-source-reference.adoc#topic-parameters

[danrosher]

checkpoints are persisted when the stream is closed, or if checkpointEvery > -1 (and then every count % checkpointEvery), otherwise the checkpoints are stored in the checkpoints hashmap, so for 'just' added docs, I think as long as is it matches the underlying query, and those docs have been soft committed (see caveat for topicstream SOLR-8709), I think they should be picked up, unless I'm completely misunderstanding ?

[epugh]

Thoughts @cpoerschke and @danrosher on this being ready for merging? Seems like a valuable bug fix! Or is there still a lack of clarity on the right behavior?

[danrosher]

This PR passes the tests in StreamDecoratorTest and StreamExpressionTest so I believe it's ready to merge

…

On Wed, 17 Aug 2022 at 12:26, Eric Pugh ***@***.***> wrote: Thoughts @cpoerschke <https://github.com/cpoerschke> and @danrosher <https://github.com/danrosher> on this being ready for merging? Seems like a valuable bug fix! Or is there still a lack of clarity on the right behavior? — Reply to this email directly, view it on GitHub <#935 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACE6HP37J3K7TBZQAZ5EZ3LVZTD5FANCNFSM53IAXYQA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[cpoerschke]

... Or is there still a lack of clarity on the right behavior?

I think the new behaviour makes sense.

Am less sure w.r.t. the documentation and/or how to describe the changes in behaviour. @epugh - feel free to jump in if you wish.

https://github.com/apache/solr/blob/5a5989e5b6164091243dd29cfe327b5eaac2cfbd/solr/solr-ref-guide/modules/query-guide/pages/stream-source-reference.adoc#topic-parameters currently says "If not set, it defaults to the highest version in the index." -- is that the highest version of documents in the index or is it alluding to checkpoints somehow? Would something like "If not set, it defaults to previously established checkpoints (if any) or otherwise the highest version in the index." be accurate and user-friendly?

[risdenk]

This looks like a good bug fix. I'm not sure why it stalled. @epugh @danrosher @cpoerschke maybe just docs?

[epugh]

i think some question about the docs. I like everything here ;-). I am going to add it to my "List of tickets to merge on Monday Oct 31st" so folks can weigh in ;-)

[epugh]

@joel-bernstein I was going to merge this today, but I realized that might conflict with your work on seperating out the streaming code? Should I wait? Do you want to add this to your list of tickets to merge once that work is done?

[github-actions]

This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!

[github-actions]

This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.