Skip to content

external index server in bgworker#355

Merged
var77 merged 5 commits intomainfrom
varik/external-index-bgworker
Nov 11, 2024
Merged

external index server in bgworker#355
var77 merged 5 commits intomainfrom
varik/external-index-bgworker

Conversation

@var77
Copy link
Copy Markdown
Collaborator

@var77 var77 commented Nov 4, 2024

  • Add external indexing server as background worker process in Lantern Extras
  • Deprecate index from file functionality in lantern and throw a message to use the new version
  • Remove old external indexing functionality from lantern/lantern_extras/lantern_cli
  • Remove tests associated with old external indexing functionality
  • Set lantern.external_index_secure=false by default, so it will correctly connect to background worker.

Copy link
Copy Markdown

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Hi! Looks like you've reached your API usage limit. You can increase it from your account settings page here: app.greptile.com/settings/usage

30 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@var77 var77 force-pushed the varik/external-index-bgworker branch from 8c40d36 to e1732ed Compare November 5, 2024 07:41
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 50.34014% with 73 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
lantern_extras/src/lib.rs 19.23% 63 Missing ⚠️
lantern_hnsw/src/hnsw/build.c 62.50% 3 Missing and 3 partials ⚠️
lantern_extras/src/daemon.rs 83.33% 2 Missing ⚠️
lantern_cli/src/daemon/mod.rs 0.00% 1 Missing ⚠️
lantern_hnsw/src/hnsw.c 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@var77 var77 requested a review from Ngalstyan4 November 5, 2024 15:23
Copy link
Copy Markdown
Contributor

@Ngalstyan4 Ngalstyan4 left a comment

Choose a reason for hiding this comment

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

small questions, but overall looks good.

/* ========================================================== */

if BackgroundWorker::sighup_received() && !ENABLE_INDEXING_SERVER.get() {
std::process::exit(1);
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.

Why are we exiting with an error code when the external indexing is not enabled?
afaik this causes a postmaster restart, no? is that expected?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is because if the bgworker exits with 0 status it won't be restarted automatically and will be unregistered from postmaster. By exiting the process we make sure that any threads created by this process will be killed and as it exits with non-zero code, it will be restarted after the configured bgw_restart_time. After boot it won't start the indexing server (as GUC will be set to false) and will wait for SIGHUP in wait_latch loop, to start in case the lantern_extras.enable_indexing_server will be set to true again.

Ref to docs: https://www.postgresql.org/docs/current/bgworker.html#:~:text=If%20bgw_restart_time%20for,itself%20has%20terminated.

PG_RETURN_NULL();
}

// todo:: remove in 0.4.3
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.

should be 0.5.1 or 0.6.0


if(buildstate->index_file_path) {
elog(ERROR,
"Importing index from file is deprecated.\n"
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.

deprecated makes it sound like the functionality still exists, but we have removed the functionality already.
Rephrase to "Importing index from file is no longer supported"

@var77 var77 force-pushed the varik/external-index-bgworker branch from 8ca0d74 to c0dfbbe Compare November 8, 2024 07:40
@var77 var77 force-pushed the varik/external-index-bgworker branch from c0dfbbe to 99625f4 Compare November 8, 2024 07:47
@var77 var77 merged commit 3574ee5 into main Nov 11, 2024
@var77 var77 deleted the varik/external-index-bgworker branch November 11, 2024 08:11
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