Conversation
janos
left a comment
There was a problem hiding this comment.
I did an early review even this PR is still in progress.
| po = k.MaxProxDisplay - 1 // why??? | ||
| } | ||
| size := bin.Size | ||
| poStr := fmt.Sprintf("%d", po) |
There was a problem hiding this comment.
To use strconv.Itoa? And in other places also.
| numConns = k.conns.Size() | ||
| numAddrs = k.addrs.Size() | ||
| connectedAddrs = make(map[string]struct{}) | ||
| connBinMap = make(map[string]string) |
There was a problem hiding this comment.
It would be good to have comments on what these strings suppose to be.
| return "\n" + strings.Join(rows, "\n") | ||
| } | ||
|
|
||
| func (k *Kademlia) MarshalJSON() (data []byte, err error) { |
There was a problem hiding this comment.
Even this method just returns the representation of Kademlia, it contains logic that should be tested. I think that testing would be beneficial here. It would even help to see what is the actual structure produced, as only maps are used and to see the structure a complete walk thought he code is needed.
I believe that tests would reveal that data argument is not even used in this method, making it not functional.
With the previous observation, I would pull out the logic of constructing maps into a separate method and test only that leaving MarshalJSON only responsible for marshalling.
|
|
||
| binMap["addresses"] = strings.Join(addresses, ",") | ||
| var binString []byte | ||
| binString, err = json.Marshal(binMap) |
There was a problem hiding this comment.
Why marshalling binMap map and assigning it as the string to m map? This will produce an escaped string in the final marshalling, instead a json structure.
This PR adds a JSON output for the Kademlia table.
It is very difficult to debug connectivity issues since we can see only 4 peers on the left side of the table and the right side does not subtract the peers on the left (showing overlapping peers).
This output has different fields for depth, each bin has a size field and list of known addresses.
The output is divided into
connectedandknownpeers. Where the peers inconnectedare subtracted fromknown. Thus, duplicates should not be shown.This can probably be improved by adding another output array of all of the known addresses (regardless of being connected or not).
Any ideas to improve would be appreciated