lens: Add ability to view raw data in metabase #1246

Merged
fyrchik merged 7 commits from a-savchuk/frostfs-node:feat/1223-lens-meta-tui into master 2024-09-05 08:03:53 +00:00
Member

Close #1223

Signed-off-by: Aleksey Savchuk a.savchuk@yadro.com

Close #1223 Signed-off-by: Aleksey Savchuk a.savchuk@yadro.com
a-savchuk requested review from acid-ant 2024-07-11 16:52:39 +00:00
a-savchuk force-pushed feat/1223-lens-meta-tui from 00c79c366f to 474639dba2 2024-07-11 17:11:24 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 77112c4ef8 to 0694451e87 2024-07-15 12:44:00 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from a96c2f4e69 to 23d1a1ed1b 2024-07-16 09:39:28 +00:00 Compare
fyrchik reviewed 2024-07-19 08:24:45 +00:00
@ -0,0 +31,4 @@
common.ExitOnErr(cmd, err)
}
func openBoltDB(cmd *cobra.Command) *bbolt.DB {
Owner

The command could be run on production databases. It would be nice to have an additional level of security.
I suggest always opening the DB in read-only mode. If we need, we will add --read-write flag later.

The command could be run on production databases. It would be nice to have an additional level of security. I suggest always opening the DB in read-only mode. If we need, we will add `--read-write` flag later.
Author
Member

I added an option to openBoltDB function and made a database open in read-only mode only for now.

I added an option to `openBoltDB` function and made a database open in read-only mode only for now.
fyrchik marked this conversation as resolved
@ -0,0 +108,4 @@
return db
}
func isEmptyBucket(b *bbolt.Bucket) bool {
Owner

It seems, the name of the function doesn't reflect its purpose.
It checks whether a bucket doesn't contain other buckets?

It seems, the name of the function doesn't reflect its purpose. It checks whether a bucket doesn't contain other buckets?
Author
Member

Removed that function as redundant.

Removed that function as redundant.
fyrchik marked this conversation as resolved
@ -0,0 +158,4 @@
}
func getSubbuckets(bp *bucketProxy) []*bucketProxy {
buckets := make([]*bucketProxy, 0)
Owner

2 argument make is used when you want to make an informed decision, otherwise default value is ok (var buckets []*bucketProxy)

2 argument `make` is used when you want to make an informed decision, otherwise default value is ok (`var buckets []*bucketProxy`)
Author
Member

Removed that function as redundant.

Removed that function as redundant.
fyrchik marked this conversation as resolved
@ -0,0 +74,4 @@
root.
SetSelectable(false).
SetExpanded(true)
Owner
  1. cobra.Command should be a part of Application, as it has .Context() method.
    This will allow you to cancel context with Ctrl+C.
  2. The default slice value should always be nil, unless you have specific reasons to use [][]byte{}.
1. `cobra.Command` should be a part of `Application`, as it has `.Context()` method. This will allow you to cancel context with Ctrl+C. 2. The default slice value should always be `nil`, unless you have specific reasons to use `[][]byte{}`.
Author
Member

Fixed.

Fixed.
fyrchik marked this conversation as resolved
@ -0,0 +148,4 @@
SetReference(BucketReference{bucket, next})
node.AddChild(child)
}
}
Owner

Does it make sense to have this in defer in the beginning of this function?
Specifically, what happens if we exit with an error on line 217?

Does it make sense to have this in defer in the beginning of this function? Specifically, what happens if we exit with an error on line 217?
Author
Member

Fixed.

Fixed.
fyrchik marked this conversation as resolved
@ -0,0 +8,4 @@
"go.etcd.io/bbolt"
)
const (
Owner

The variable should be a default value (defaultBufferSize) and the size of the buffer should be a parameter of your function.
It is important, because it directly affects perfomance and responsiveness.

The variable should be a default value (`defaultBufferSize`) and the size of the buffer should be a parameter of your function. It is important, because it directly affects perfomance and responsiveness.
Author
Member

Fixed.

Fixed.
fyrchik marked this conversation as resolved
@ -0,0 +50,4 @@
transform func(key, value []byte) any,
) (<-chan any, error) {
buffer := make(chan any, bufferSize)
Owner

It is better to use db.View, unless you have specific reasons to manage tx lifecycle by hand. There are lot's of things that could go wrong.

It is better to use `db.View`, unless you have specific reasons to manage tx lifecycle by hand. There are lot's of things that could go wrong.
Author
Member

Fixed it.

Fixed it.
fyrchik marked this conversation as resolved
@ -0,0 +55,4 @@
return f
}
func getItemsRecursively(f *tview.Flex) []tview.Primitive {
Owner

The default values for slice should be nil, like var items []tview.Primitive.

The default values for slice should be `nil`, like `var items []tview.Primitive`.
Author
Member

That component is redundant now. At first, I though I'll have UI with main window split, one view on the left and another on the right. The component was created to make focus change between two views possible.

Because of possible small terminal size (80 symbols width and 25 high) and a lot of data to show I've decided each view to be displayed on a separate page.

I removed that component.

That component is redundant now. At first, I though I'll have UI with main window split, one view on the left and another on the right. The component was created to make focus change between two views possible. Because of possible small terminal size (80 symbols width and 25 high) and a lot of data to show I've decided each view to be displayed on a separate page. I removed that component.
fyrchik marked this conversation as resolved
@ -0,0 +61,4 @@
count := f.GetItemCount()
for i := 0; i < count; i++ {
switch item := f.GetItem(i).(type) {
case *tview.Flex:
Owner

Depending on how much items to you have, this will scale badly (because you allocate a slice, which you then append and GC).
A better API would be providing the slice to which you append as a parameter to getItemsRecursively (nil by default).

Depending on how much items to you have, this will scale badly (because you allocate a slice, which you then append and GC). A better API would be providing the slice to which you append as a parameter to `getItemsRecursively` (`nil` by default).
Author
Member

Removed that component.

Removed that component.
fyrchik marked this conversation as resolved
@ -0,0 +29,4 @@
func NewMultipageView() *MultipageView {
view := &MultipageView{
Box: tview.NewBox(),
Owner

The default value should be nil (omit the field).

The default value should be nil (omit the field).
Author
Member

Omit all the fields except Box. That is a common approach in tview library: Box component has a rich interface that's inherited by all other components, that interface facilitates set of custom input handlers, draw of a component's frame and title, and etc.

Here's an example from tview.

Omit all the fields except `Box`. That is a common approach in `tview` library: `Box` component has a rich interface that's inherited by all other components, that interface facilitates set of custom input handlers, draw of a component's frame and title, and etc. Here's an [example](https://github.com/rivo/tview/blob/b0a7293b81308ab0e44c2757f8922683a3d659df/list.go#L94) from `tview`.
fyrchik marked this conversation as resolved
@ -0,0 +32,4 @@
items: make([]*MultipageItem, 0),
nextItemFunc: func() *MultipageItem { return nil },
Owner

It is ok to omit default values.

It is ok to omit default values.
Author
Member

Fixed it.

Fixed it.
@ -0,0 +52,4 @@
func (v *MultipageView) GetNextItemFunc() func() *MultipageItem {
return v.nextItemFunc
}
Owner

Why the getter is called GetNextItemFunc(), but the setter is SetPopulateFunc()?

Why the getter is called `GetNextItemFunc()`, but the setter is `SetPopulateFunc()`?
Author
Member

Fixed it.

Fixed it.
@ -0,0 +19,4 @@
return func(key, value []byte) (nextKey, nextValue []byte, view *View) {
acc := &View{}
for _, formatter := range formatters {
key, value, view = formatter(key, value)
Owner

Not important here, as the items are low in size, but it is usually good to use strings.Builder for string construction, as it doesn't allocate intermediate slices.
You can then construct acc right before you return.
Another option is to revise the whole API, formatter could know about the place is writes to (like accepting io.Writer).

Not important here, as the items are low in size, but it is usually good to use `strings.Builder` for string construction, as it doesn't allocate intermediate slices. You can then construct `acc` right before you return. Another option is to revise the whole API, formatter could know about the place is writes to (like accepting `io.Writer`).
Author
Member

Fixed.

Fixed.
@ -0,0 +29,4 @@
var acc error
for _, h := range hs {
view, next, err = h(key, value)
if err == nil {
Owner

return view, next, nil?

`return view, next, nil`?
Author
Member

Fixed.

Fixed.
@ -0,0 +41,4 @@
// TODO: change the message
// TODO: make predefined error ???
return nil, nil, errors.New("has no prefix")
}
Owner

Isn't it bytes.HasPrefix from the stdlib?

Isn't it `bytes.HasPrefix` from the stdlib?
Author
Member

Fixed.

Fixed.
@ -0,0 +79,4 @@
return key, value[32:], nil
}
func TerminationValidator(key, value []byte) (nextKey, nextValue []byte, err error) {
Owner

Should be a global error, similar to others.
Also, I do not understand the message. How about sth with extra data or expected EOF, got ...?

Should be a global error, similar to others. Also, I do not understand the message. How about sth with `extra data` or `expected EOF, got ...`?
Author
Member

Fixed.

Fixed.
@ -51,7 +51,6 @@ func (np *Processor) processNewEpoch(ev netmapEvent.NewEpoch) bool {
if epoch > 0 && np.alphabetState.IsAlphabet() { // estimates are invalid in genesis epoch
err = np.containerWrp.StartEstimation(prm)
Owner

Please, rebase and remove it from this PR. If it is still in master, make a PR.

Please, rebase and remove it from this PR. If it is still in master, make a PR.
Author
Member

Done, I created a PR #1265.

Done, I created a PR #1265.
a-savchuk force-pushed feat/1223-lens-meta-tui from 7c6e43e89e to 04e01ebd4c 2024-07-19 08:46:19 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 18fd55aaf7 to c4e3f5e894 2024-07-22 12:49:08 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from a7f16b4639 to 85e6f48b4e 2024-07-23 07:24:04 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 85e6f48b4e to 11f1e0c4f1 2024-07-23 09:02:01 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 11f1e0c4f1 to 5b67b98ed2 2024-07-23 13:35:13 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from f664d6cac6 to 1a3ce7fc11 2024-07-26 10:31:54 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from c98aba178c to 973e6fdd4f 2024-07-29 07:54:13 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 5355b69bdf to e7bb68d1cf 2024-08-06 07:31:19 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from cc49dfcb97 to 28fce53fc8 2024-08-12 07:13:05 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from eaf379deeb to da3b22f32f 2024-08-12 13:03:52 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from da3b22f32f to 16a9c7458e 2024-08-12 13:13:05 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from f687a54666 to 642af83a81 2024-08-12 15:36:11 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 46f657797f to 71c1adcacc 2024-08-13 22:04:53 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 71c1adcacc to 381f882db3 2024-08-14 08:28:18 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 1e2bd4f666 to 4e4b3d84c1 2024-08-14 21:56:59 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 4e4b3d84c1 to 356c690c49 2024-08-15 07:26:31 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 356c690c49 to 2b2201f60c 2024-08-15 07:32:16 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 2b2201f60c to ffa7bf44ec 2024-08-15 08:18:38 +00:00 Compare
acid-ant reviewed 2024-08-16 07:51:31 +00:00
@ -0,0 +46,4 @@
}
func runTUI(cmd *cobra.Command) error {
// tview.Styles.PrimaryTextColor = tcell.ColorBlack
Member

Please remove commented code.

Please remove commented code.
Author
Member

Removed.

Removed.
a-savchuk force-pushed feat/1223-lens-meta-tui from ffa7bf44ec to b737910813 2024-08-16 08:34:48 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from b737910813 to 16d52d0085 2024-08-16 13:07:59 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 16d52d0085 to 7305e8cba1 2024-08-16 13:16:40 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 7305e8cba1 to 55c631d8ca 2024-08-16 13:21:45 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 55c631d8ca to ba2df86612 2024-08-16 13:34:35 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from ba2df86612 to c9044fd1c0 2024-08-16 14:18:55 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from c9044fd1c0 to 08af2adbb4 2024-08-16 14:29:10 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 08af2adbb4 to 9782d2479f 2024-08-16 14:47:30 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 9782d2479f to 7317c43d5f 2024-08-19 06:50:21 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 7317c43d5f to bac3ed0bcd 2024-08-19 15:02:58 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from bac3ed0bcd to 408ab046b9 2024-08-20 07:01:51 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 408ab046b9 to f45e852b8f 2024-08-20 07:16:55 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from f45e852b8f to 4f941a8492 2024-08-20 07:21:01 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 4f941a8492 to 1af2e6cf11 2024-08-20 07:25:34 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 1af2e6cf11 to 57a36ff0e5 2024-08-20 07:50:18 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 57a36ff0e5 to 65d6e1fa2b 2024-08-20 08:10:32 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 65d6e1fa2b to 15da026da5 2024-08-20 08:18:00 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 15da026da5 to c19caebef1 2024-08-20 08:27:55 +00:00 Compare
a-savchuk requested review from storage-core-developers 2024-08-20 08:33:58 +00:00
a-savchuk requested review from storage-core-committers 2024-08-20 08:33:59 +00:00
a-savchuk changed title from WIP: lens: Add ability to view raw data in `metabase` to lens: Add ability to view raw data in `metabase` 2024-08-20 08:34:06 +00:00
dstepanov-yadro reviewed 2024-08-20 13:15:49 +00:00
@ -0,0 +13,4 @@
)
var tuiCMD = &cobra.Command{
Use: "tui",

TUI is a tool name. For command it is better something like metabase explore IMHO.

`TUI` is a tool name. For command it is better something like `metabase explore` IMHO.
Author
Member

Changed the command name.

Changed the command name.
dstepanov-yadro approved these changes 2024-08-20 13:24:39 +00:00
a-savchuk force-pushed feat/1223-lens-meta-tui from c19caebef1 to ca2ba8e22c 2024-08-22 12:08:35 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from ca2ba8e22c to b67ddea4da 2024-08-22 12:17:05 +00:00 Compare
aarifullin reviewed 2024-08-26 11:09:56 +00:00
@ -0,0 +161,4 @@
var locked []oid.ID
var i uint
for ; i < count; i++ {
Member

For me, when a variable is assigned out of for-statement, I expect that it's used somewhere else :) How about this?

for i := uint(0); i < count; i++ {
}
For me, when a variable is assigned out of `for`-statement, I expect that it's used somewhere else :) How about this? ```go for i := uint(0); i < count; i++ { } ```
Author
Member

That's a good point. Changed it everywhere.

That's a good point. Changed it everywhere.
aarifullin marked this conversation as resolved
dstepanov-yadro approved these changes 2024-08-26 11:20:10 +00:00
aarifullin reviewed 2024-08-26 11:28:01 +00:00
@ -0,0 +38,4 @@
exitOnZero("must have owners", *numOwners)
exitOnZero("path to metabase not specified", *path)
if *numAttributesPerObj > *numAttributes {
Member

If you introduce exitIfFalse (instead exitOnZero), then you also can use this one-line manner to exit the programm on error :)

exitIfFalse("must have payloads", *numPayloads > 0)
exitIfFalse("must have attributes", *numAttributes > 0)
exitIfFalse("must have owners", *numOwners > 0)
exitIfFalse("path to metabase not specified", len(*path) > 0)
exitIfFalse("object can't have more attributes than available", *numAttributesPerObj <= *numAttributes)

WDYT?

If you introduce `exitIfFalse` (instead `exitOnZero`), then you also can use this one-line manner to exit the programm on error :) ```go exitIfFalse("must have payloads", *numPayloads > 0) exitIfFalse("must have attributes", *numAttributes > 0) exitIfFalse("must have owners", *numOwners > 0) exitIfFalse("path to metabase not specified", len(*path) > 0) exitIfFalse("object can't have more attributes than available", *numAttributesPerObj <= *numAttributes) ``` WDYT?
Author
Member

I think it's awesome. I've changed it.

I think it's awesome. I've changed it.
aarifullin marked this conversation as resolved
aarifullin reviewed 2024-08-26 11:37:37 +00:00
@ -0,0 +39,4 @@
}
func tuiFunc(cmd *cobra.Command, _ []string) {
common.ExitOnErr(cmd, runTUI(cmd))
Member

You pass cmd to runTUI. So, you can declare runTUI without returning error.
You have already exit with common.ExitOnErr(cmd, err) when openDB is failed. Let's make it for all errors

func runTUI(cmd *cobra.Command) {
   /*...*/
   common.ExitOnErr("prompt error: %w", ui.WithPrompt(initialPrompt))
   /*...*/
   common.ExitOnErr("application run error: %w", app.Run())
}
You pass `cmd` to `runTUI`. So, you can declare `runTUI` without returning `error`. You have already exit with `common.ExitOnErr(cmd, err)` when `openDB` is failed. Let's make it for all errors ```go func runTUI(cmd *cobra.Command) { /*...*/ common.ExitOnErr("prompt error: %w", ui.WithPrompt(initialPrompt)) /*...*/ common.ExitOnErr("application run error: %w", app.Run()) } ```
Author
Member

That makes sense especially your comment about error handling after opening a database. However, I have several defers need to be called, so I think it'd be easier to handle them in runTUI and then exit with an error in tuiFunc. Please feel free to correct me if I'm wrong.

I've changed error handling after opening a database, it now returns an error to the calling function.

That makes sense especially your comment about error handling after opening a database. However, I have several `defer`s need to be called, so I think it'd be easier to handle them in `runTUI` and then exit with an error in `tuiFunc`. Please feel free to correct me if I'm wrong. I've changed error handling after opening a database, it now returns an error to the calling function.
aarifullin marked this conversation as resolved
aarifullin reviewed 2024-08-26 11:38:30 +00:00
@ -0,0 +13,4 @@
)
var tuiCMD = &cobra.Command{
Use: "tui",
Member

What does TUI stand for? Can you leave a comment, please

What does `TUI` stand for? Can you leave a comment, please
Author
Member

I've changed the command name based on that comment. Now it looks like this

$ frostfs-lens meta explore --path=meta.db

Also I've made the command help message more clear.

I've changed the command name based on [that comment](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1246#issuecomment-48252). Now it looks like this ```bash $ frostfs-lens meta explore --path=meta.db ``` Also I've made the command help message more clear.
aarifullin marked this conversation as resolved
a-savchuk force-pushed feat/1223-lens-meta-tui from b67ddea4da to da99d57e91 2024-08-30 14:15:14 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from da99d57e91 to e7af9beb11 2024-08-30 14:15:44 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from e7af9beb11 to 3570b7ea19 2024-08-30 14:51:04 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 3570b7ea19 to 6a8df845c1 2024-08-30 15:20:57 +00:00 Compare
a-savchuk force-pushed feat/1223-lens-meta-tui from 6a8df845c1 to 37a903d27a 2024-08-30 15:52:07 +00:00 Compare
aarifullin approved these changes 2024-09-02 10:28:08 +00:00
aarifullin left a comment
Member

This is one of the most useful and in-demand tool. Nice!

This is one of the most useful and in-demand tool. Nice!
fyrchik approved these changes 2024-09-05 08:03:48 +00:00
fyrchik merged commit 7768a482b5 into master 2024-09-05 08:03:53 +00:00
a-savchuk removed review request for acid-ant 2024-09-05 08:05:55 +00:00
a-savchuk deleted branch feat/1223-lens-meta-tui 2024-09-05 08:06:06 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1246
No description provided.