add online filter #51
Conversation
xmonader
requested review from
mariobassem,
ashraffouda,
rawdaGastan,
Omarabdul3ziz,
AbdelrahmanElawady,
sameh-farouk and
Eslam-Nawara
as code owners
|
|
||
| // Create a response with the node data plus online status | ||
| response := gin.H{ | ||
| "node": node, |
There was a problem hiding this comment.
I don't like this return.
| lastSeenFormatted = node.LastSeen.Format(time.RFC3339) | ||
| } | ||
|
|
||
| response[i] = gin.H{ |
There was a problem hiding this comment.
I don't like this return too
There was a problem hiding this comment.
This appears to reintroduce the changes proposed in PR #46, adding the online filter. It’s also based directly on the development branch, so I assume it completely replaces the earlier PR. That said, it’s worth noting that a few aspects handled in the original PR are now missing, most notably:
- The migration for the last_seen field, which should be stored in the DB.
- The timestamp hint validation, which I believe is important.
This is an initial look, I'll go through the code now
| @@ -78,7 +100,27 @@ func (db *Database) GetUptimeReports(nodeID uint64, start, end time.Time) ([]Upt | |||
| } | |||
|
|
|||
| func (db *Database) CreateUptimeReport(report *UptimeReport) error { | |||
| return db.gormDB.Create(report).Error | |||
| // Start a transaction | |||
| tx := db.gormDB.Begin() | |||
There was a problem hiding this comment.
Instead of controlling the transaction manually, Gorm offers a simple flow https://gorm.io/docs/transactions.html#Transaction
|
|
||
| // Update the node's LastSeen field | ||
| now := time.Now() | ||
| if err := tx.Model(&Node{}).Where("node_id = ?", report.NodeID).Update("last_seen", now).Error; err != nil { |
There was a problem hiding this comment.
The uptime report submitted by the node includes the timestamp at which the uptime was recorded. Shouldn't we validate that this timestamp falls within an acceptable range and use it instead? While it will typically align with the current time, edge cases may occur, which is likely why the node includes the timestamp in the report in the first place.
| return tx.Error | ||
| } | ||
|
|
||
| // Create the uptime report |
There was a problem hiding this comment.
To correctly bind the expected JSON data from Zos nodes to the UptimeReportRequest struct in Go, we should adjust the struct fields to match the actual data format being sent. Specifically:
The Uptime field should be defined as int64 instead of time.Duration, and then converted to a time.Duration (by multiplying it by time.Second) inside the handler.
The Timestamp field should also be defined as int64 to represent a Unix timestamp in seconds.
This is necessary because Zos sends Uptime as the number of seconds and Timestamp as a Unix timestamp in seconds. However, Go expects time.Duration in nanoseconds and parses time.Time from ISO 8601 or RFC3339 formatted strings by default. This mismatch would lead to incorrect or failed binding unless explicitly handled.
|
It was meant to be from your branch indeed. closed. |
Continuation of #46