PP-838 Support for logging via Syslog in PBS

[tejas.pai]

@agrawalravi90

• Now that get_log_type() doesn’t take any arguments, you should remove the text:
  " Input

syslog - by default None

Done

@agrawalravi90

• If get_log_type() is meant to be only used by log_match() then please make it an internal function to the class which contains log_match and prefix it with underscores.

Done

@agrawalravi90

In the 3rd and 4th rows of the table, you’ve mentioned the value of PBS_SYSLOG as ‘set’, please change it to ‘1’.

This is done because PBS_SYSLOG value might be between 1 to 7. Therefore I have added this as ‘set’ and not 1

@agrawalravi90

• I think the following bullets are not needed as they get explained by the table:
  "
  b) If PBS_SYSLOG is not set, we will locally in code set variable PBS_SYSLOG=0

 c) If  PBS_LOCALLOG is not set, we will locally in code set variable PBS_LOCALLOG=1

Removed this

@agrawalravi90

I think you should add another row which explains what happens if both of them are unset, so the row will look something like “unset unset Local_log local_log(x,y)”

Done

@agrawalravi90

I also think that the column ‘Return Value for the log_match()’ is confusing. That table is explaining the behavior of the function get_log_type(), so it’s confusing to see what the function log_match() will return here. So, I’d recommend deleting this column and instead just explain in log_match() that it will return log lines from the local log if it matches from both.

Removed this

@agrawalravi90

If get_syslog_lines() is only intended to be used by LogUtils.log_lines(), then please make it an internal function of PBSLogUtils class and prefix it with underscores.

Done

@agrawalravi90

Seems like get_rsyslog_priorites() and get_rsyslog_files() are internal functions which get used by get_syslog_lines(), so please rename them with leading underscores.

Done

[agrawalravi90]

@tejas.pai

This is done because PBS_SYSLOG value might be between 1 to 7. Therefore I have added this as ‘set’ and not 1

The maybe change it to “1-7” or “1 to 7” ? it can be set to 0 as well, that’s the only reason I’m bugging you about this.

And I think PBSSyslogUtils is not needed anymore as it only contains internal functions. The functions should be moved to the class where they are used.

Thanks again for being so patient.

[tejas.pai]

@agrawalravi90

The maybe change it to “1-7” or “1 to 7” ? it can be set to 0 as well, that’s the only reason I’m bugging you about this.

Done

@agrawalravi90

And I think PBSSyslogUtils is not needed anymore as it only contains internal functions. The functions should be moved to the class where they are used.

Added the existing functions to class PBSLogUtils

[agrawalravi90]

Just one last thing Tejas: _get_log_type() should be moved to the class which contains log_match() since log_match() is the function which uses it. So, please move it to the class PBSService

[tejas.pai]

@agrawalravi90

Just one last thing Tejas: _get_log_type() should be moved to the class which contains log_match() since log_match() is the function which uses it. So, please move it to the class PBSService

Moved this to class PBSService

[agrawalravi90]

Looks good Tejas, thanks for making all the changes