Skip to content

rename pindex functions to not include import#8098

Merged
torcolvin merged 3 commits intomainfrom
import-renames
Mar 9, 2026
Merged

rename pindex functions to not include import#8098
torcolvin merged 3 commits intomainfrom
import-renames

Conversation

@torcolvin
Copy link
Collaborator

prepare for use of functions to be shared by resync and import

prepare for use of functions to be shared by resync and import
@torcolvin torcolvin requested a review from RIT3shSapata March 6, 2026 16:02
Copilot AI review requested due to automatic review settings March 6, 2026 16:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR prepares the pindex helper functions for use by both resync and import by renaming them to remove the "import"-specific naming. It also improves error messages and updates log statements.

Changes:

  • Renamed private helpers (registerImportPindexImplMutexregisterPindexImplMutex, getListenerImportDestgetCbgtDest, newImportPIndexImplnewPIndexImpl) to be generic
  • Improved the error message when unmarshalling SGFeedIndexParams to include the problematic value (with base.MD() wrapping)
  • Downgraded the "No dest found for indexParams" log from InfofCtx to DebugfCtx

if err != nil {
// This error can occur when a stale index definition hasn't yet been removed from the plan (e.g. on update to db config)
base.InfofCtx(ctx, base.KeyDCP, "No importDest found for indexParams - usually an obsolete index pending removal. %v", err)
base.DebugfCtx(ctx, base.KeyDCP, "No dest found for indexParams - usually an obsolete index pending removal. %v", err)
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The log level for "No dest found for indexParams" was downgraded from InfofCtx to DebugfCtx. While the comment says this is "usually an obsolete index pending removal", it is still an error path where err != nil. Downgrading to Debug may make it harder to diagnose real issues in production environments where debug logging is not enabled. At minimum, if this case is truly expected during normal operation, the comment or log message should clarify when it represents an actual problem versus a transient state.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was intentional.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
RIT3shSapata
RIT3shSapata previously approved these changes Mar 9, 2026
Copy link
Contributor

@RIT3shSapata RIT3shSapata left a comment

Choose a reason for hiding this comment

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

shouldn't the filename also be changed?

@torcolvin
Copy link
Collaborator Author

shouldn't the filename also be changed?

I'll change the filename now, the reason I didn't originally is that changing the filename will make the diff in github hard to read.

@torcolvin torcolvin merged commit a9b6672 into main Mar 9, 2026
27 of 28 checks passed
@torcolvin torcolvin deleted the import-renames branch March 9, 2026 18:49
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.

3 participants