[jira] [Commented] (SOLR-16924) Restore: Have RESTORECORE set the UpdateLog state
[ https://issues.apache.org/jira/browse/SOLR-16924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17845130#comment-17845130 ] David Smiley commented on SOLR-16924: - > (Funnily enough, this is actually an idea I got from you! I'll add the link > in here if I can dig it up...) Yes I do remember now :) it makes sense. It'd be helpful if classes distinctly "V1" had javadocs saying so. I'll move the logic. Probably not worth a JIRA. When I see a package "handler", I'm thinking request handlers, thus API (and may include implementation). Even the package-info.java says it's for SolrRequestHandlers. But this class doesn't end in Handler so okay, it's not a handler. I'm good with it. > Restore: Have RESTORECORE set the UpdateLog state > -- > > Key: SOLR-16924 > URL: https://issues.apache.org/jira/browse/SOLR-16924 > Project: Solr > Issue Type: Improvement >Reporter: David Smiley >Priority: Minor > Fix For: 9.5 > > Time Spent: 50m > Remaining Estimate: 0h > > This is a refactoring improvement designed to simplify & clarify a step in > collection restores. One of the final phases of RestoreCmd (collection > restore) is to call REQUESTAPPLYUPDATES on each newly restored replica in > order to transition the state of the UpdateLog to ACTIVE (not actually to > apply updates). The underlying call on the UpdateLog could instead be done > inside RESTORECORE at the end with explanatory comments as to the intent. I > think this makes more sense that RESTORECORE finish with its UpdateLog ready. > And it's strange/curious to see requests in the cluster to apply updates > from an updateLog when there is none to do! Adding clarifying comments is > important. > See my comment: > https://issues.apache.org/jira/browse/SOLR-12065?focusedCommentId=17751792=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17751792 > I think there isn't any back-compat concern. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-16924) Restore: Have RESTORECORE set the UpdateLog state
[
https://issues.apache.org/jira/browse/SOLR-16924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17845105#comment-17845105
]
Jason Gerlowski commented on SOLR-16924:
bq. got very confused for a while until I realized we have two RestoreCore
classes [...] Wow; ok! The first implements the V2 API and calls the second,
the V1 API.
Not exactly. 'admin.api.RestoreCore' is our V2 API. But 'handler.RestoreCore'
isn't the v1 API - it's an implementation class that contains a lot of the
logic to do the restore itself. It's used by both the v1 and v2 API endpoints.
The actual v1 entrypoint, such as it is, is CoreAdminHandler (which calls
RestoreCoreOp).
So, to summarize the logic flow for both v1 and v2:
*v2*
{code}
admin.api.RestoreCore.restoreCore (v2 entrypoint)
|-> handler.RestoreCore.doRestore (actual implementation logic)
{code}
*v1*
{code}
CoreAdminHandler.handleRequestBody (v1 entrypoint)
|-> RestoreCoreOp.execute
|---> admin.api.RestoreCore.restoreCore
|-> handler.RestoreCore.doRestore (actual implementation logic)
{code}
As you pointed out, the v1 code path calls the v2 codepath (which then calls
the business logic). Which is surprising/unusual, but it's something we did
very intentionally: it ensures that v1 and v2 endpoints for an API remain in
sync (i.e. it prevents developers from adding a param to v1 without updating
v2). And by having v1 consume the v2 code, it sets us up to transition away
from v1 at some point in the future without needing to modify the v2 endpoint
or feature logic. (Funnily enough, this is actually an idea I got from you!
I'll add the link in here if I can dig it up...)
Moving on to your actual questions:
bq. Are there any problems or bugs here?
Not that I can see.
There might be some maintenance-cost improvements we can make: move
'handler.RestoreCore' to a different package or rename it to make it clearer
that it's not "api" code maybe?
bq. Like, should the change in this PR be placed at the end of V1 instead?
If by "at the end of v1" you mean the "handler.RestoreCore" business-logic
class - then probably?
For functionality like restore-core that has a dedicated business-logic class,
I'd expect 'admin.api.RestoreCore' to be mostly "api" logic (e.g. validating
inputs) and for all logic pertaining to restoring to go into
"handler.RestoreCore". That's not a bug, to be clear - but it's a nice
division-of-responsibilities to try and maintain.
bq. Should the RestoreCore classes be merged?
I don't think so, nope. Two reasons:
1. It's valuable to keep "api" concerns and business logic separate.
2. The business logic in handler.RestoreCore is reused by Solr for several
other restore-like operations as well (e.g. "install-shard"). We wouldn't want
to tie it to any particular API since we have 2 or 3 different APIs that call
into this business logic.
Hopefully that helps clarify; lmk if I missed any questions or it still doesn't
make sense?
> Restore: Have RESTORECORE set the UpdateLog state
> --
>
> Key: SOLR-16924
> URL: https://issues.apache.org/jira/browse/SOLR-16924
> Project: Solr
> Issue Type: Improvement
>Reporter: David Smiley
>Priority: Minor
> Fix For: 9.5
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> This is a refactoring improvement designed to simplify & clarify a step in
> collection restores. One of the final phases of RestoreCmd (collection
> restore) is to call REQUESTAPPLYUPDATES on each newly restored replica in
> order to transition the state of the UpdateLog to ACTIVE (not actually to
> apply updates). The underlying call on the UpdateLog could instead be done
> inside RESTORECORE at the end with explanatory comments as to the intent. I
> think this makes more sense that RESTORECORE finish with its UpdateLog ready.
> And it's strange/curious to see requests in the cluster to apply updates
> from an updateLog when there is none to do! Adding clarifying comments is
> important.
> See my comment:
> https://issues.apache.org/jira/browse/SOLR-12065?focusedCommentId=17751792=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17751792
> I think there isn't any back-compat concern.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-16924) Restore: Have RESTORECORE set the UpdateLog state
[
https://issues.apache.org/jira/browse/SOLR-16924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17844085#comment-17844085
]
David Smiley commented on SOLR-16924:
-
[~gerlowskija] I could use your insights on something here, please. I am
looking back at this months later and got very confused for a while until I
realized we have two RestoreCore classes, one in
{{org.apache.solr.handler.admin.api}} and the other in
{{{}org.apache.solr.handler{}}}. Wow; ok! The first implements the V2 API and
calls the second, the V1 API. The change in this PR was placed at the end of
the V2 API without consideration of the ambiguity. Uh oh! Thus it would seem
the change will not take effect for the V1 API {*}but{*}, I see in SOLR-16490
that RestoreCoreOp (yet another layer below the CoreAdminHandler but above the
real impl) calls the V2 RestoreCore. Wow again; I didn't expect that! I
wonder then, is V1 RestoreCore invoked in any other code path? I see two –
{{ReplicationHandler.restore()}} and {{{}InstallCoreData.installCoreData(){}}}.
Are there any problems or bugs here? Like, _should_ the change in this PR be
placed at the end of V1 instead? Isn't it wrong for CoreAdminHandler to be
calling v2 stuff? On second thought, I could rationalize that as we transition
the migration. Should the RestoreCore classes be merged?
> Restore: Have RESTORECORE set the UpdateLog state
> --
>
> Key: SOLR-16924
> URL: https://issues.apache.org/jira/browse/SOLR-16924
> Project: Solr
> Issue Type: Improvement
>Reporter: David Smiley
>Priority: Minor
> Fix For: 9.5
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> This is a refactoring improvement designed to simplify & clarify a step in
> collection restores. One of the final phases of RestoreCmd (collection
> restore) is to call REQUESTAPPLYUPDATES on each newly restored replica in
> order to transition the state of the UpdateLog to ACTIVE (not actually to
> apply updates). The underlying call on the UpdateLog could instead be done
> inside RESTORECORE at the end with explanatory comments as to the intent. I
> think this makes more sense that RESTORECORE finish with its UpdateLog ready.
> And it's strange/curious to see requests in the cluster to apply updates
> from an updateLog when there is none to do! Adding clarifying comments is
> important.
> See my comment:
> https://issues.apache.org/jira/browse/SOLR-12065?focusedCommentId=17751792=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17751792
> I think there isn't any back-compat concern.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-16924) Restore: Have RESTORECORE set the UpdateLog state
[ https://issues.apache.org/jira/browse/SOLR-16924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17773514#comment-17773514 ] ASF subversion and git services commented on SOLR-16924: Commit b885c8abbb023e1becb9bb1cc12592cb6c9b377d in solr's branch refs/heads/main from David Smiley [ https://gitbox.apache.org/repos/asf?p=solr.git;h=b885c8abbb0 ] SOLR-16924: needn't retain back-compat > Restore: Have RESTORECORE set the UpdateLog state > -- > > Key: SOLR-16924 > URL: https://issues.apache.org/jira/browse/SOLR-16924 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) >Reporter: David Smiley >Priority: Minor > Time Spent: 50m > Remaining Estimate: 0h > > This is a refactoring improvement designed to simplify & clarify a step in > collection restores. One of the final phases of RestoreCmd (collection > restore) is to call REQUESTAPPLYUPDATES on each newly restored replica in > order to transition the state of the UpdateLog to ACTIVE (not actually to > apply updates). The underlying call on the UpdateLog could instead be done > inside RESTORECORE at the end with explanatory comments as to the intent. I > think this makes more sense that RESTORECORE finish with its UpdateLog ready. > And it's strange/curious to see requests in the cluster to apply updates > from an updateLog when there is none to do! Adding clarifying comments is > important. > See my comment: > https://issues.apache.org/jira/browse/SOLR-12065?focusedCommentId=17751792=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17751792 > I think there isn't any back-compat concern. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-16924) Restore: Have RESTORECORE set the UpdateLog state
[ https://issues.apache.org/jira/browse/SOLR-16924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17773511#comment-17773511 ] ASF subversion and git services commented on SOLR-16924: Commit c3f7e733ced594a4190e9fc920edc383ac749ff7 in solr's branch refs/heads/branch_9x from julia-maimone [ https://gitbox.apache.org/repos/asf?p=solr.git;h=c3f7e733ced ] SOLR-16924: RESTORECORE: make UpdateLog ACTIVE without requiring REQUESTAPPLYUPDATES (#1965) RESTORECORE now sets the UpdateLog to ACTIVE state instead of requiring a separate REQUESTAPPLYUPDATES call in Collection restore. The latter will still happen in 9.x for backwards-compatibility. - Co-authored-by: Julia Maimone Co-authored-by: David Smiley > Restore: Have RESTORECORE set the UpdateLog state > -- > > Key: SOLR-16924 > URL: https://issues.apache.org/jira/browse/SOLR-16924 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) >Reporter: David Smiley >Priority: Minor > Time Spent: 50m > Remaining Estimate: 0h > > This is a refactoring improvement designed to simplify & clarify a step in > collection restores. One of the final phases of RestoreCmd (collection > restore) is to call REQUESTAPPLYUPDATES on each newly restored replica in > order to transition the state of the UpdateLog to ACTIVE (not actually to > apply updates). The underlying call on the UpdateLog could instead be done > inside RESTORECORE at the end with explanatory comments as to the intent. I > think this makes more sense that RESTORECORE finish with its UpdateLog ready. > And it's strange/curious to see requests in the cluster to apply updates > from an updateLog when there is none to do! Adding clarifying comments is > important. > See my comment: > https://issues.apache.org/jira/browse/SOLR-12065?focusedCommentId=17751792=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17751792 > I think there isn't any back-compat concern. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-16924) Restore: Have RESTORECORE set the UpdateLog state
[ https://issues.apache.org/jira/browse/SOLR-16924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17773512#comment-17773512 ] ASF subversion and git services commented on SOLR-16924: Commit 8c08ee7c21f50e0a7574e15ecffb89aceea03659 in solr's branch refs/heads/branch_9x from David Smiley [ https://gitbox.apache.org/repos/asf?p=solr.git;h=8c08ee7c21f ] SOLR-16924: needn't retain back-compat > Restore: Have RESTORECORE set the UpdateLog state > -- > > Key: SOLR-16924 > URL: https://issues.apache.org/jira/browse/SOLR-16924 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) >Reporter: David Smiley >Priority: Minor > Time Spent: 50m > Remaining Estimate: 0h > > This is a refactoring improvement designed to simplify & clarify a step in > collection restores. One of the final phases of RestoreCmd (collection > restore) is to call REQUESTAPPLYUPDATES on each newly restored replica in > order to transition the state of the UpdateLog to ACTIVE (not actually to > apply updates). The underlying call on the UpdateLog could instead be done > inside RESTORECORE at the end with explanatory comments as to the intent. I > think this makes more sense that RESTORECORE finish with its UpdateLog ready. > And it's strange/curious to see requests in the cluster to apply updates > from an updateLog when there is none to do! Adding clarifying comments is > important. > See my comment: > https://issues.apache.org/jira/browse/SOLR-12065?focusedCommentId=17751792=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17751792 > I think there isn't any back-compat concern. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-16924) Restore: Have RESTORECORE set the UpdateLog state
[ https://issues.apache.org/jira/browse/SOLR-16924?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17772438#comment-17772438 ] ASF subversion and git services commented on SOLR-16924: Commit 8740b07efa8e78ab9ad1842cfd836e23b2cf6fd8 in solr's branch refs/heads/main from julia-maimone [ https://gitbox.apache.org/repos/asf?p=solr.git;h=8740b07efa8 ] SOLR-16924: RESTORECORE: make UpdateLog ACTIVE without requiring REQUESTAPPLYUPDATES (#1965) RESTORECORE now sets the UpdateLog to ACTIVE state instead of requiring a separate REQUESTAPPLYUPDATES call in Collection restore. The latter will still happen in 9.x for backwards-compatibility. - Co-authored-by: Julia Maimone Co-authored-by: David Smiley > Restore: Have RESTORECORE set the UpdateLog state > -- > > Key: SOLR-16924 > URL: https://issues.apache.org/jira/browse/SOLR-16924 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) >Reporter: David Smiley >Priority: Minor > Time Spent: 50m > Remaining Estimate: 0h > > This is a refactoring improvement designed to simplify & clarify a step in > collection restores. One of the final phases of RestoreCmd (collection > restore) is to call REQUESTAPPLYUPDATES on each newly restored replica in > order to transition the state of the UpdateLog to ACTIVE (not actually to > apply updates). The underlying call on the UpdateLog could instead be done > inside RESTORECORE at the end with explanatory comments as to the intent. I > think this makes more sense that RESTORECORE finish with its UpdateLog ready. > And it's strange/curious to see requests in the cluster to apply updates > from an updateLog when there is none to do! Adding clarifying comments is > important. > See my comment: > https://issues.apache.org/jira/browse/SOLR-12065?focusedCommentId=17751792=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17751792 > I think there isn't any back-compat concern. -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
