log neighbors to database #13
Conversation
|
@SimonSK - did you consider using an Object Relational Mapping framework such as SQLAlchemy? It will require a little bit of initial config, but ultimately should make it simpler to deal with database interactions aka you wouldn't have to write raw SQL. SQLAlchemy also provides db connection pool management. |
|
@zzma A quick search shows different opinions about ORM on go, but yea, I will look into it and change to that if codes become simpler. As for the db connection pool, I believe the go builtin |
|
Gotcha - sorry I brain farted and thought we were using python. I'm not saying that we should definitely use an ORM, but just wanted to make you aware and see if it you think it would be helpful |
|
@zzma fyi, there are some changes made to the log message formats and the db table schemes in later commit that will be part of another PR. I will post that PR after this one is merged. |
|
@SimonSK what's the status of this PR? There are still some unaddressed comments |
| } | ||
|
|
||
| func (t *udp) addNodeMetaInfo(nodeid string, hash string) { | ||
| _, err := t.addNodeMetaInfoStmt.Exec(nodeid, hash) |
There was a problem hiding this comment.
While technically correct, this is a bit confusing, since it is not clear how many arguments and what arguments should be passed to addNodeMetaInfoStmt.Exec(). It also is different for addNeighborStmt.Exec()
I think an ORM would help with this
There was a problem hiding this comment.
Added this to the TODO list (#15). I implemented all other sql related tasks for p2p hello and eth status in later commits, so I will try to refactor this at once after merging everything done so far.
| @@ -286,6 +342,22 @@ func newUDP(priv *ecdsa.PrivateKey, c conn, natm nat.Interface, nodeDBPath strin | |||
| func (t *udp) close() { | |||
| close(t.closing) | |||
| t.conn.Close() | |||
|
|
|||
| // close prepared sql statements | |||
There was a problem hiding this comment.
the Go way of doing this is to use defer at the point in the code when the object is being opened, so that you don't have to "remember" to do close something at the end. You could refactor this logic into a function closeStmt and call defer closeStmt(t.addNeighborStmt) or something
There was a problem hiding this comment.
Right. I didn't use defer because the statements are prepared when the UDP server is started (in newUDP) and there is a separate logic for closing the server. If I used defer in newUDP, wouldn't the statement be closed as soon as the start logic returns? Anyways, I will make this into a generic closing function as suggested, but I will make the change after the merge to avoid further conflicts. I made a TODO list in #15
| func (t *udp) addNeighbor(nodeid string, ip string, tcpPort uint16, udpPort uint16, unixTime float64) { | ||
| _, err := t.addNeighborStmt.Exec(nodeid, ip, tcpPort, udpPort, unixTime, unixTime) | ||
| if err != nil { | ||
| log.Proto("MYSQL", "action", "execute AddNeighbor statement", "result", "fail", "err", err) |
There was a problem hiding this comment.
It seems like we should log errors at a more severe log level, probably log.Error
There was a problem hiding this comment.
Yes. I pushed them down to debug/trace level in future commits. This will be part of another PR.
There was a problem hiding this comment.
I meant that it should be a more severe logging level - we don't want things to fail quietly
NEIGHBORSpackets are logged in theneighborsandnode_meta_infotables