Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

external index server in bgworker #355

Merged
merged 5 commits into from
Nov 11, 2024
Merged

Conversation

var77
Copy link
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

@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
Copy link

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

@@ -486,6 +486,12 @@ static void BuildIndex(Relation heap, Relation index, IndexInfo *indexInfo, ldb_

InitBuildState(buildstate, heap, index, indexInfo);

if(buildstate->index_file_path) {
elog(ERROR,
"Importing index from file is deprecated.\n"
Copy link
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
67 of 68 checks passed
@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