search

sni / lmd

Livestatus Multitool Daemon - Create livestatus federation from multiple sources
https://labs.consol.de/omd/packages/lmd/
GNU General Public License v3.0
42 stars 31 forks source link

Make peers aware of the columns for the DataTables #50

Closed danirod closed 5 years ago

danirod commented 5 years ago

This pull request adds a field called ColumnsMap to the DataTable object that is used by peers during initialization. This field is a map of type map[int]int, and it maps a ColumnIndex (as described by the Table, such as the numbers you find in Table.ColumnIndexes or in Column.Index, to the actual index to access when extracting data from rows in the DataTable data (DataTable.Data).

Why

As explained in #41, this is a required step to avoid offsetting errors, because if the table has optional fields that are not present for a peer of a particular kind, each row in the datatable will have less columns than the sum of static and dynamic columns defined in the Table object backing that particular datatable.

If a column that has an index greater than the index of an optional column that has been stripped for the datatable because of this is accessed, the column index for the DataTable data will not actually match the column index available in the ColumnIndex for the Table definition

Therefore, I need to have some data structure that I can use to query the following question: "which index of a datatable data row holds the value for the column whose index in the object table definition is X?", and then access that particular column.

Alternate proposals

I considered having a more complex map, or an index similar to the ColumnsIndex used in Table, which uses objects of type Columns, but I think that would make the model more complicated with redundant data.

I also considered letting the peers compute the actual indexes on runtime instead of caching data as a field. Something like moving the call to BuildResponseIndexes to compute the indexes values from response.go to peer.go:

https://github.com/sni/lmd/blob/7302d294350e6612ea8b61a0047d6487f21fe7d0/lmd/response.go#L87

So when a request is received, rather than passing the numerical indexes of the columns to (*Response).BuildLocalResponse, the columns are still unresolved strings. It's up to the peer to map the column string into an appropiate column value, similar to how (*Table).GetInitialKeys does so:

https://github.com/sni/lmd/blob/0ffe74514e122f3da45543109842be22c943e317/lmd/objects.go#L213-L222

However, I believe that this adds some extra overhead to each request as it now has to compute a lot of columns and that could make the request slower to be handled.

Anyway, since it's my first patch and because this change updates the data model for the program, I'm open to suggestions or requests for change on this.

sni commented 5 years ago

good catch, let me have a look at the broken tests, they seem unrelated to this change.

sni commented 5 years ago

could you please rebase your branch, then the tests should pass.

danirod commented 5 years ago

Sure, done. Let's see if Travis likes it more now.

danirod commented 5 years ago

Hm, make citest complains. Let me check if it's a broken test in my branch.

danirod commented 5 years ago

Well, indeed, it seems my diff breaks virtual tables such as comments or tables. I think I have an idea on why this happens, there is a branching in CreateObjectByType that maybe has to be done differently.

danirod commented 5 years ago

Okay, everything is fixed now, sorry :sweat_smile:. I did only take into account the first condition on this if statement, so I assumed that tables entering this condition would never have keys, and no keys = no map to compute.

https://github.com/sni/lmd/blob/048efa9abbb5bfff013fc179686a3cc73b8899a0/lmd/peer.go#L1535

sni commented 5 years ago

great, thank you very much.