Martin Peřina has posted comments on this change.

Change subject: fklsnr: Introduce standalone fence_kdump listener
......................................................................


Patch Set 13:

(6 comments)

http://gerrit.ovirt.org/#/c/27201/13/packaging/services/ovirt-fence-kdump-listener/db.py
File packaging/services/ovirt-fence-kdump-listener/db.py:

Line 199:                 status,                          # v_status
Line 200:                 json.dumps(address),             # v_address
Line 201:             ),
Line 202:         )
Line 203:         return res[0]['upsertkdumpstatusforip'] == 1 if res else False
> how can res be nothing? I expect it to be either error (exception) or # of 
Well, you're probably right, I think we can remove this failsafe ...
Line 204: 
Line 205:     def update_heartbeat(self):
Line 206:         return self._db_mgr.call_procedure(
Line 207:             name='UpsertExternalVariable',


http://gerrit.ovirt.org/#/c/27201/13/packaging/services/ovirt-fence-kdump-listener/listener.py
File packaging/services/ovirt-fence-kdump-listener/listener.py:

Line 189:                     )
Line 190:                 )
Line 191:             # remove finished sessions (engine will remove them from 
db)
Line 192:             if session['status'] == self.SESSION_STATE_CLOSED:
Line 193:                 del self._sessions[session['address']]
> I am unsure you can delete the session while iterating.
You're right, I forgot that Python 3 will return iterator instead of new list 
when calling values() ...
Line 194: 
Line 195:     def _heartbeat(self):
Line 196:         if self._interval_finished(
Line 197:                 interval=self._heartbeatInterval,


Line 257:                 if address not in self._sessions:
Line 258:                     session = self._create_session(
Line 259:                         status=self.SESSION_STATE_DUMPING,
Line 260:                         address=address,
Line 261:                         updated=datetime.datetime.utcnow(),
> I think you can put this now() as default within session every time you cre
Done
Line 262:                         dirty=False,
Line 263:                     )
Line 264:                 self._sessions[session['address']] = session
Line 265: 


Line 260:                         address=address,
Line 261:                         updated=datetime.datetime.utcnow(),
Line 262:                         dirty=False,
Line 263:                     )
Line 264:                 self._sessions[session['address']] = session
> this should be within the if scope, bad python not having scopes!
Done
Line 265: 
Line 266:             self._afterFirstDbSync = True
Line 267: 
Line 268:     def _db_sync(self):


Line 279:                 self._db_connection_valid = True
Line 280:                 try:
Line 281:                     self._heartbeat()
Line 282:                     self._save_sessions()
Line 283:                     self._load_sessions()
> any reason why load is after save? not that it is that important... but in 
Well, memory session contains more recent data, so when I save them to db 
first, I will probably read less unfinished sessions during load ...

But the difference is not big :-)
Line 284:                 except db.DbException as e:
Line 285:                     self.logger.debug(
Line 286:                         (
Line 287:                             "Error during synchronization with 
database, "


Line 298:                             "Database connection is not available, "
Line 299:                             "synchronization will be postponed."
Line 300:                         )
Line 301:                     )
Line 302:                 self._lastDbConnReopen = datetime.datetime.utcnow()
> lastDbConenctionAttempt ?
Done
Line 303: 
Line 304:     def run(self):
Line 305:         while True:
Line 306:             (data, address) = self._recvfrom()


-- 
To view, visit http://gerrit.ovirt.org/27201
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieec3bad47bbba860a52a9ff4e2eb7f61277f4e36
Gerrit-PatchSet: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Martin Peřina <mper...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to