Skip to content

Introduce a new stat which indicates a bleve index's convergence#2152

Merged
Thejas-bhat merged 3 commits intomasterfrom
newStats
Feb 28, 2025
Merged

Introduce a new stat which indicates a bleve index's convergence#2152
Thejas-bhat merged 3 commits intomasterfrom
newStats

Conversation

@Thejas-bhat
Copy link
Member

@Thejas-bhat Thejas-bhat commented Feb 25, 2025

index_bgthreads_active indicates whether the background routines that maintain the index are busy doing some work. This means that the index hasn't converged to a steady state and there could be potential file segment merges or in-memory segment flushes still remaining.
This stat is beneficial for the application layer to get better insight as to whether the index is doing some background work (which can have implications on the system's resource utilisation) or not.

@Thejas-bhat Thejas-bhat changed the title Introduce a new stat index_converging Introduce a new stat which indicates a bleve index's convergence Feb 25, 2025
if rootEpoch, ok := m["CurRootEpoch"].(uint64); ok {
if lastMergedEpoch, ok := m["LastMergedEpoch"].(uint64); ok {
if lastPersistedEpoch, ok := m["LastPersistedEpoch"].(uint64); ok {
m["index_converging"] = !(lastMergedEpoch == rootEpoch && lastPersistedEpoch == rootEpoch)
Copy link
Member

Choose a reason for hiding this comment

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

maybe call index_idle which will be true if index is idle (when lastMergedEpoch == rootEpoch && lastPersistedEpoch == rootEpoch)

Copy link
Member Author

Choose a reason for hiding this comment

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

i see. is there any specific reason why you'd want it that way? from what i gather based on the suggestion, it is just speaking the stat in a different way without adding any extra information to the stat relative what's defined right now (which can be renamed as index_active i guess?)

Copy link
Member

Choose a reason for hiding this comment

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

ya either index_active or index_idle ... index_converging doesnt make sense

Copy link
Member

Choose a reason for hiding this comment

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

Agree with Rahul here @Thejas-bhat - index_idle makes the most sense to me.

Copy link
Member Author

@Thejas-bhat Thejas-bhat Feb 25, 2025

Choose a reason for hiding this comment

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

sure but i will have to keep the stat as "index_active" at the cbft level because of how the stats have been initialised with a default value 0. so in a multi partition setup, index_idle means that all partitions MUST be idle so its an AND logic with the default value being 0. however, if we talk in terms of an index being active, atleast one partition being active serves the logic and the changes aren't that messy.

this is mainly why i chose index_active instead of index_idle, to keep the stats consistent between both bleve and cbft.

please let me know your thoughts on it.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, let's keep the naming consistent between the two though - so if you choose index_active let's go with it.

@abhinavdangeti abhinavdangeti added this to the v2.5.0 milestone Feb 25, 2025
@abhinavdangeti
Copy link
Member

@Thejas-bhat I suppose we're getting another patch here with the bg name?

@abhinavdangeti
Copy link
Member

Also, I saw your bool to uint conversion downstream - let's just make it an int here itself.

@Thejas-bhat
Copy link
Member Author

Also, I saw your bool to uint conversion downstream - let's just make it an int here itself.

so the main reason why I kept it as bool here and didn't bother to change it is because its semantically more correct to keep the stat as a bool at the bleve level.
The boolToUint() conversion is necessary mainly due to how cbft plays around with the stats init, tracks and displays it (which is the application level concern I think and something bleve doesn't have to worry about in my opinion).
However, if we are okay with maintaining it as an int over here itself I can do that, its just that it won't be immediately understandable if the bleve community user sees it as 1 or 0.

@Thejas-bhat Thejas-bhat merged commit be70bab into master Feb 28, 2025
9 checks passed
@abhinavdangeti abhinavdangeti deleted the newStats branch February 28, 2025 15:30
ns-codereview pushed a commit to couchbase/cbft that referenced this pull request Feb 28, 2025
- introduce a new stat `index_bgthreads_active` which indicates whether
all the partitions belong to an index on a specific node are in the
process of reaching a steady state (due to background routines' activity).

- if the value is false, its safe to assume that every partition of that
index has reached a steady state (no merger/persister activity)

- blevesearch/bleve#2152

Change-Id: I9603164a7cac6737de2daf5004589d00f2ad8833
Reviewed-on: https://review.couchbase.org/c/cbft/+/223958
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: <thejas.orkombu@couchbase.com>
Reviewed-by: Abhi Dangeti <abhinav@couchbase.com>
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