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.

Sign up for GitHub

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

Jump to bottom

Update indexing settings #4868

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Update indexing settings #4868

wants to merge 4 commits into from

Conversation

rdettai
Copy link
Contributor

@rdettai rdettai commented Apr 15, 2024 •
edited

Description

  • Add the indexing settings to the update APIs
  • Split the update APIs to one route per updatable field to avoid side effects (e.g not specifying the retention policy drops it)
  • Simplify the CLI to be a very basic client for the APIs that read the input body from files

How was this PR tested?

Unit tests and validated that swagger works fine.

@rdettai rdettai self-assigned this Apr 15, 2024
@rdettai rdettai added the enhancement New feature or request label Apr 15, 2024
@rdettai rdettai linked an issue Apr 15, 2024 that may be closed by this pull request
Merge policy update footgun #4697
Open
@rdettai rdettai mentioned this pull request Apr 15, 2024
Open
rdettai
rdettai commented Apr 15, 2024
View reviewed changes
Copy link
Contributor Author

@rdettai rdettai left a comment

Choose a reason for hiding this comment

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

A few notes on this early draft

quickwit/quickwit-control-plane/src/model/mod.rs
@@ -54,7 +54,7 @@ use tracing::{info, instrument, warn};
#[derive(Default, Debug)]
pub(crate) struct ControlPlaneModel {
index_uid_table: FnvHashMap<IndexId, IndexUid>,
index_table: FnvHashMap<IndexUid, IndexMetadata>,
index_source_table: FnvHashMap<IndexUid, HashMap<SourceId, SourceConfig>>,
Copy link
Contributor Author

@rdettai rdettai Apr 15, 2024 •
edited

Choose a reason for hiding this comment

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

I refactored the ControlPlaneModel because:

  • it is more explicit to only store the sources in the state as the rest of the IndexMetadata isn't relevant
  • if we kept the entire IndexMetadata in the state, we would need to also proxy the update calls through the control plane to keep it in sync

Copy link
Member

Choose a reason for hiding this comment

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

Please, revert. When we will support doc mapping updates, we certainly want to know about the whole IndexMetadata and proxy updates via the cp.

Copy link
Contributor Author

@rdettai rdettai May 7, 2024 •
edited

Choose a reason for hiding this comment

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

Ok I'll revert. Don't you think it would be better to have a smaller struct that only contains the data that is really useful to the control plane? It would make it easier to read what the control plane does. Nit, it would also make it possible to update the control plane state only for updates that really impact the used state.

quickwit/quickwit-metastore/src/tests/index.rs Outdated Show resolved Hide resolved
quickwit/quickwit-integration-tests/src/tests/index_update_tests.rs Outdated Show resolved Hide resolved
@rdettai rdettai changed the title Indexing-settings-update Update indexing settings Apr 15, 2024
Base automatically changed from index-update to main April 15, 2024 12:42
@rdettai rdettai force-pushed the indexing-settings-update branch 4 times, most recently from 85ac73c to b79c240 Compare April 16, 2024 16:35
@rdettai rdettai requested a review from guilload April 17, 2024 14:13
@rdettai rdettai marked this pull request as ready for review April 17, 2024 14:13
@rdettai rdettai requested a review from trinity-1686a May 6, 2024 09:11
@rdettai rdettai mentioned this pull request May 6, 2024
Draft
9 tasks
guilload
guilload reviewed May 6, 2024
View reviewed changes
quickwit/quickwit-cli/src/index/update.rs Outdated Show resolved Hide resolved
quickwit/quickwit-cli/src/index/update.rs Outdated Show resolved Hide resolved
quickwit/quickwit-cli/src/index/update.rs Outdated Show resolved Hide resolved
docs/reference/rest-api.md Outdated Show resolved Hide resolved
docs/reference/rest-api.md Outdated Show resolved Hide resolved
quickwit/quickwit-config/src/lib.rs Outdated Show resolved Hide resolved
quickwit/quickwit-control-plane/src/model/mod.rs
@@ -54,7 +54,7 @@ use tracing::{info, instrument, warn};
#[derive(Default, Debug)]
pub(crate) struct ControlPlaneModel {
index_uid_table: FnvHashMap<IndexId, IndexUid>,
index_table: FnvHashMap<IndexUid, IndexMetadata>,
index_source_table: FnvHashMap<IndexUid, HashMap<SourceId, SourceConfig>>,
Copy link
Member

Choose a reason for hiding this comment

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

Please, revert. When we will support doc mapping updates, we certainly want to know about the whole IndexMetadata and proxy updates via the cp.

quickwit/quickwit-metastore/src/metastore/mod.rs Outdated Show resolved Hide resolved
quickwit/quickwit-metastore/src/metastore/mod.rs Outdated Show resolved Hide resolved
quickwit/quickwit-proto/protos/quickwit/metastore.proto Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guilload guilload guilload left review comments

@trinity-1686a trinity-1686a Awaiting requested review from trinity-1686a

At least 1 approving review is required to merge this pull request.

Assignees

@rdettai rdettai

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge policy update footgun
2 participants
@rdettai @guilload