Skip to content

Conversation

@hmmr
Copy link

@hmmr hmmr commented Oct 28, 2025

This implements martinsumner#478.

When called here, it returns a map containing reportable items described in the issue above.

(dev2@127.0.0.1)12> riak_kv_vnode:vnode_status(riak_core_vnode_manager:all_index_pid(riak_kv_vnode)).
[{1347321821914426127719021955160323408745312813056,
  [{backend_status,riak_kv_leveled_backend,
                   #{ledger_cache_size => 4,
                     journal_last_compaction_result => {0,0.0},
                     get_sample_count => 0,get_body_time => 0,
                     head_sample_count => 0,head_rsp_time => 0,
                     put_sample_count => 0,put_prep_time => 0,put_ink_time => 0,
                     put_mem_time => 0}},
   {vnodeid,<<"}ÍyرYdå">>},
   {counter,140003},
   {counter_lease,150000},
   {counter_lease_size,10000},
   {counter_leasing,false}]},
...

Not all items can be available at the time of calling, in which case the backend_status will not include the corresponding keys.

In riak_control, the results will show as follows:
Screenshot_2025-10-29_01-11-07
Screenshot_2025-10-29_01-11-33

@hmmr
Copy link
Author

hmmr commented Oct 28, 2025

Marking as WIP pending work on the last item ("Mean level for recent fetches").

@hmmr hmmr changed the title WIP leveled_bookie:status/1 leveled_bookie:status/1 Oct 29, 2025
@martinsumner martinsumner moved this to Todo in OpenRiak 3.4.1 Jan 7, 2026
@martinsumner martinsumner moved this from Todo to Ready For Review in OpenRiak 3.4.1 Jan 7, 2026
@martinsumner martinsumner self-requested a review January 7, 2026 13:25
@martinsumner
Copy link

Can we add a test to the ct tests, or perhaps just extend one of the tests in basic_SUITE.

It would be good to verify that the results returned are as expected. Some may just need to be range checked, but you can get the PIDs of the Inker/Penciller via leveled_bookie:book_returnactors/1 to absolutely verify things like the penciller cache size.

Obviously fails the coverage check at the moment as book_status/1 is never called:

|------------------------|------------|
  |                module  |  coverage  |
  |------------------------|------------|
  |        leveled_bookie  |       99%  |
  |           leveled_cdb  |      100%  |
  |         leveled_codec  |      100%  |
  |        leveled_ebloom  |      100%  |
  |          leveled_eval  |      100%  |
  |        leveled_filter  |      100%  |
  |          leveled_head  |      100%  |
  |        leveled_iclerk  |      100%  |
  |     leveled_imanifest  |      100%  |
  |         leveled_inker  |      100%  |
  |           leveled_log  |      100%  |
  |       leveled_monitor  |       97%  |
  |        leveled_pclerk  |      100%  |
  |     leveled_penciller  |      100%  |
  |     leveled_pmanifest  |      100%  |
  |          leveled_pmem  |      100%  |
  |        leveled_runner  |      100%  |
  |         leveled_setop  |      100%  |
  |           leveled_sst  |      100%  |
  |      leveled_sstblock  |      100%  |
  |        leveled_tictac  |      100%  |
  |          leveled_tree  |      100%  |
  |          leveled_util  |      100%  |
  |------------------------|------------|
  |                 total  |       99%  |
  |------------------------|------------|

There are tests in basic_SUITE that prompt journal compaction, so you cna follow the pattern to check results are expected after a compaction.

@hmmr hmmr changed the title leveled_bookie:status/1 WIP leveled_bookie:status/1 Jan 15, 2026
@martinsumner
Copy link

The PUT will call leveled_monitor:maybe_time/2 - so sometimes it will be timed, and sometimes not. So I'm not sure that the test will pass reliably.

I do think it should try a number of operations, so that we can check all the values change to the right order of magnitude. There are a lot of load functions, or you can add a fetch and validation of the book status into one of the other tests.

Also, what happened to reporting the journal compaction scores and results?


-spec book_status(pid()) -> proplists:proplist().
%% @doc
%% Return a proplist conteaining the following items:

Choose a reason for hiding this comment

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

containing

book_headstatus(Pid) ->
gen_server:call(Pid, head_status, infinity).

-spec book_status(pid()) -> proplists:proplist().

Choose a reason for hiding this comment

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

A proplist or a map?

put_mem_time => PT#bookie_put_timings.mem_time,
fetch_count_by_level => FCL
}.

Choose a reason for hiding this comment

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

If there has been no compaction, is there no report on compaction? Would it not be better to consistently sure all keys exist, but have them set to a default value when they haven't occurred.

code_change(_OldVsn, State, _Extra) ->
{ok, State}.

enriched_bookie_status(#state{
Copy link

@martinsumner martinsumner Jan 20, 2026

Choose a reason for hiding this comment

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

dialyzer spec required.

By default, in leveled, we try to avoid passing the LoopState to functions outside of the standard handle_call/caast/info functions. I would probably do this within the handle_call rather than have it as a separate function - as it is just a function based on knowledge of #state, and there's not separate testing of the function. [in this case there would be no need for a dialyzer spec]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready For Review

Development

Successfully merging this pull request may close these issues.

2 participants