Created Czar::FamilyMapRead function to be called on map update notif…#1034
Conversation
iagaponenko
left a comment
iagaponenko
left a comment
There was a problem hiding this comment.
I would highly recommend using Copilot to prescreen the PRs.
| /// been read and the request can be safely ignored. | ||
| TIMEPOINT _latestFamilyUpdate; | ||
| /// If more than this time has past, check the family map. | ||
| std::chrono::seconds _familyMapmaxUpdateWait{600}; |
There was a problem hiding this comment.
For the sake of consistency, when using the Camel-case naming convention:
_familyMapMaxUpdateWait
| /// If the requestTime is > _latestFamilyUpdate (start of last read), the family map is out probably out | ||
| /// of date. | ||
| /// @return true if the family map was read. | ||
| bool FamilyMapRead(TIMEPOINT const requestTime); |
There was a problem hiding this comment.
Two questions/comments:
- Is this method supposed to be
public? - The name of the method should begin with lowercase as per: https://developer.lsst.io/cpp/style.html
There was a problem hiding this comment.
It's meant to be public as it can be called from anywhere when there's a notification that the tables have been changed.
First character is now lower case.
|
|
||
| bool Czar::FamilyMapRead(TIMEPOINT const requestTime) { | ||
| string const funcN("Czar::FamilyMapRead"); | ||
| bool familyMapRead = false; |
There was a problem hiding this comment.
Could you pick a different name for the variable? It's basically (module lower/upper case first letter) the same as the name of the enclosing method. Something like:
bool mapWasUpdated = false;
| if (requestTime >= _latestFamilyUpdate || CLOCK::now() - _latestFamilyUpdate > _familyMapmaxUpdateWait) { | ||
| /// Check database for changes in worker chunk assignments and aliveness | ||
| try { | ||
| auto familyUpdateStart = CLOCK::now(); |
There was a problem hiding this comment.
Why is this variable here? It doesn't seem to be used anywhere. Did you mean to measure the performance of the table read operation?
There was a problem hiding this comment.
Since the read may take a while (multiple milliseconds), the time used for comparisons needs to be before the read starts. That guarantees that updates aren't missed.
| LOGS(_log, LOG_LVL_DEBUG, funcN << " start0"); | ||
| // Only one thread at a time should do this. | ||
| lock_guard familyUpdateLock(_latestFamilyUpdateMtx); | ||
| if (requestTime >= _latestFamilyUpdate || CLOCK::now() - _latestFamilyUpdate > _familyMapmaxUpdateWait) { |
There was a problem hiding this comment.
I think you have a bug here:
CLOCK::now() - _latestFamilyUpdate > _familyMapmaxUpdateWait
Perhaps you meant to say this?
CLOCK::now() - _familyMapmaxUpdateWait > _latestFamilyUpdate
There was a problem hiding this comment.
I might be wrong here.
There was a problem hiding this comment.
latest = 8, now = 20, maxWait = 10
20 - 8 > 10 looks ok
fritzm
left a comment
fritzm
left a comment
There was a problem hiding this comment.
LGTM, too -- thanks!
+1 on recommendation to have copilot review. Why not?
3 participants
When the tables are updated, the notification just needs to call
Czar::FamilyMapRead.