solr icon indicating copy to clipboard operation
solr copied to clipboard

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

Open danrosher opened this issue 3 years ago • 5 comments

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:

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

danrosher avatar Jul 11 '22 16:07 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

danrosher avatar Jul 15 '22 14:07 danrosher

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 avatar Jul 18 '22 14:07 cpoerschke

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?

epugh avatar Aug 17 '22 11:08 epugh

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 https://github.com/apache/solr/pull/935#issuecomment-1217883013, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACE6HP37J3K7TBZQAZ5EZ3LVZTD5FANCNFSM53IAXYQA . You are receiving this because you were mentioned.Message ID: @.***>

danrosher avatar Aug 17 '22 14:08 danrosher

... 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?

cpoerschke avatar Aug 24 '22 15:08 cpoerschke

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 avatar Oct 27 '22 19:10 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?

epugh avatar Oct 31 '22 14:10 epugh

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 [email protected] mailing list. Thank you for your contribution!

github-actions[bot] avatar Feb 19 '24 00:02 github-actions[bot]