Martin Peřina has posted comments on this change.

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


Patch Set 6:

(12 comments)

http://gerrit.ovirt.org/#/c/27201/6/ovirt-engine.spec.in
File ovirt-engine.spec.in:

Line 404: Requires:     %{name} = %{version}-%{release}
Line 405: Requires:     %{name}-lib >= %{version}-%{release}
Line 406: Requires:     java
Line 407: Requires:     logrotate
Line 408: Requires:     python-dateutil
> as far as I can see this is not required.
Done
Line 409: Requires:     python-psycopg2
Line 410: 
Line 411: %if %{ovirt_install_systemd}
Line 412: Requires(post):               systemd


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

Line 66:     def __exit__(self, exc_type, exc_value, traceback):
Line 67:         self._socket.close()
Line 68: 
Line 69:     def _now(self):
Line 70:         return datetime.datetime.now(dateutil.tz.tzutc())
> datetime.datetime.utcnow()
Done
Line 71: 
Line 72:     def _total_seconds(self, td):
Line 73:         return (
Line 74:             td.microseconds + (td.seconds + td.days * 24 * 3600) * 
10**6


Line 71: 
Line 72:     def _total_seconds(self, td):
Line 73:         return (
Line 74:             td.microseconds + (td.seconds + td.days * 24 * 3600) * 
10**6
Line 75:         ) / 10**6
> please use the magic of:
Done
Line 76: 
Line 77:     def _interval_finished(self, interval, last):
Line 78:         return (
Line 79:             last is None


Line 181:                         address=entry['address'][0],
Line 182:                     )
Line 183:                 )
Line 184:                 entry['status'] = self.SESSION_STATE_DUMPING
Line 185:                 self._sessions[entry['address'][0]] = entry
> 1. please use (address, port) always, per what we discussed.
1. Done
Line 186: 
Line 187:             elif entry['status'] == self.SESSION_STATE_DUMPING:
Line 188:                 self.logger.debug(
Line 189:                     'dumping %s',


Line 204:             self.logger.debug('Exception',  exc_info=True)
Line 205: 
Line 206:     def _house_keeping_sessions(self):
Line 207:         for session in self._sessions.values():
Line 208:             received = self._now()
> please move out the now from loop
Done
Line 209: 
Line 210:             if self._interval_finished(
Line 211:                     interval=self._sessionExpirationTime,
Line 212:                     last=session['received']


Line 261:             try:
Line 262:                 new_heartbeat = self._now()
Line 263: 
Line 264:                 self._dao.update_heartbeat(
Line 265:                     heartbeat=new_heartbeat,
> please use database now() no need to involve python time
Done
Line 266:                 )
Line 267: 
Line 268:                 self._lastHeartbeat = new_heartbeat
Line 269:                 self.logger.debug('heartbeat')


Line 281:             # is always 'dumping'
Line 282:             record['status'] = self.SESSION_STATE_DUMPING
Line 283:             # address is stored as string, convert back to tuple
Line 284:             record['address'] = tuple(record['address'])
Line 285:             self._sessions[record['address'][0]] = record
> the tracking should be based on (address, port) and initialized in similar 
Done
Line 286: 
Line 287: 


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

Line 75:         self._config = configfile.ConfigFile(
Line 76:             (
Line 77:                 self._defaults,
Line 78:                 config.ENGINE_VARS,
Line 79:                 config.ENGINE_FKLSNR_VARS,
> you should not mix two configurations at one load.
Done
Line 80:             ),
Line 81:         )
Line 82: 
Line 83:         self._checkInstallation(


Line 88:         consoleLog = open(os.devnull, "w+")
Line 89:         return (consoleLog, consoleLog)
Line 90: 
Line 91:     def daemonContext(self):
Line 92:         db_manager = db.ConnectionManager(
> with ....
Done
Line 93:             host=self._config.get('ENGINE_DB_HOST'),
Line 94:             port=self._config.get('ENGINE_DB_PORT'),
Line 95:             database=self._config.get('ENGINE_DB_DATABASE'),
Line 96:             username=self._config.get('ENGINE_DB_USER'),


Line 95:             database=self._config.get('ENGINE_DB_DATABASE'),
Line 96:             username=self._config.get('ENGINE_DB_USER'),
Line 97:             password=self._config.get('ENGINE_DB_PASSWORD'),
Line 98:             secured=self._config.getboolean('ENGINE_DB_SECURED'),
Line 99:             secure_validation=(
> we do:
Done
Line 100:                 
self._config.getboolean('ENGINE_DB_SECURED_VALIDATION')
Line 101:             ),
Line 102:         )
Line 103: 


Line 98:             secured=self._config.getboolean('ENGINE_DB_SECURED'),
Line 99:             secure_validation=(
Line 100:                 
self._config.getboolean('ENGINE_DB_SECURED_VALIDATION')
Line 101:             ),
Line 102:         )
> ) as db_manager:
Done
Line 103: 
Line 104:         with db_manager:
Line 105:             dao = db.EngineDao(connection_manager=db_manager)
Line 106: 


Line 116:                         
config_values['FenceKdumpListenerHeartbeatInterval']
Line 117:                     ),
Line 118:                     session_expiration_time=int(
Line 119:                         config_values['KdumpFinishedTimeout']
Line 120:                     ),
> if you load these explicitly in dao, why don't you convert presentation the
Done
Line 121:             ) as server:
Line 122:                 server.run()
Line 123: 
Line 124: 


-- 
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: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Peřina <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Martin Peřina <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to