HPCC-33142 Distinguish between id and sequence number in SysInfoLogger #19367
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33142 Jirabot Action Result: |
dali/base/sysinfologger.cpp
Outdated
| #define ATTR_TIMESTAMP "@ts" | ||
| #define ATTR_AUDIENCE "@audience" | ||
| #define ATTR_CLASS "@class" | ||
| #define ATTR_CODE "@code" | ||
| #define ATTR_HIDDEN "@hidden" | ||
| #define ATTR_SOURCE "@source" | ||
|
|
||
| #define NON_SYSLOGMSG_MASK 0x8000000000 |
There was a problem hiding this comment.
trivial: static constexpr unsigned __int64 nonSysInfoLogMsgMask = 0x8000000000; .. would be more modern/compiler friendly. Or "sysLogInfoIsStaticMask"?
dali/base/sysinfologger.hpp
Outdated
| @@ -89,4 +89,8 @@ SYSINFO_API unsigned deleteLogSysInfoMsg(IConstSysInfoLoggerMsgFilter * msgFilte | |||
| SYSINFO_API bool deleteLogSysInfoMsg(unsigned __int64 msgId, const char *source=nullptr); | |||
| SYSINFO_API unsigned deleteOlderThanLogSysInfoMsg(bool visibleOnly, bool hiddenOnly, unsigned year, unsigned month, unsigned day, const char *source=nullptr); | |||
|
|
|||
| /* makeMessageId - Create a message id from date, sequence number and nonSysInfoLogMsg flag to uniquely identify a message | |||
| - nonSysInfoLogMsg flag is used to identify whether or not the message managed by SysInfoLogger */ | |||
There was a problem hiding this comment.
trivial:
whether or not the message managed by SysInfoLogger
whether or not the message is managed by SysInfoLogger
dali/base/sysinfologger.cpp
Outdated
| { | ||
| return id<<21 | year<<9 | month<<5 | day; | ||
| return (nonSysInfoLogMsg?NON_SYSLOGMSG_MASK:0) | seqN<<21 | year<<9 | month<<5 | day; |
There was a problem hiding this comment.
not new/minor: bits 9-20 inclusive reserved for year (12 bits), so max year is 4095?
Not that it's ever going to an issue :) , but potentially confusing to reader, should cope with any 4 digit year, and needs 14 bits for that. so would have to be seqN<<23 I think.
Arguably might be better to more explicitly acknowledge that the sequence number is 32bit only and shift it to the top 4 bytes, and use the spare bits in the lower unsigned space for everything else:
return seqN<<32 | (nonSysInfoLogMsg?nonSysInfoLogMsgMask:0) | year<<9 | month<<5 | day;
where nonSysInfoLogMsgMask uses (e.g.) 23th bit
Probably worth adding constexpr for day/month/year masks, with comments to bit range for clarify.
There was a problem hiding this comment.
- I'm not sure that max year 4095 is confusing. 9999 is another arbitrary upper limit just like 4095. (It is just as valid to have year 10000 year 9999!)
- If supporting all 4 digit years is important, we need the nonSysInfoLogMsgMask at 24 bit (not bit 23)
- If we have the nonSysInfoLogMsgMask as the 32 bit, it would allow other bits between 25-31 to be be available for other uses.
There was a problem hiding this comment.
you're right 9999 is just as arbitrary - I think as a long as it's clear. Add comments that using 12 bits and what upper limit is and it'll be fine (I think supporting 0-9999 would be conventional though).
If supporting all 4 digit years is important, we need the nonSysInfoLogMsgMask at 24 bit (not bit 23)
yes 24th bit:
0-4 = day
5-8 = month
9-22 = if 14 bits for year [ currently 9-20 (12 bits) ]
23-31 = for other meta, e.g. 1 bit to signify static messages.
32-63 = upper 32 bits for sequence number.
would be worth adding this as a comment block (near the mask definition) and/or adding masks for other for others together with the comments.
dali/base/sysinfologger.cpp
Outdated
| { | ||
| return id<<21 | year<<9 | month<<5 | day; | ||
| // bits 0-4 = day, bits 5-8 = month, bits 9-23 = year |
There was a problem hiding this comment.
these ranges are inclusive. 9-23 = 15 bits - allowing year range of 0 - 32767.
There was a problem hiding this comment.
@shamser - looks good. 1 comment (https://github.com/hpcc-systems/HPCC-Platform/pull/19367/files#r1895937587) re. year range.
Please squash.
shamser
force-pushed
the
issue33142
branch
2 times, most recently
from
ec55bcd to
968121e
Compare
dali/base/sysinfologger.cpp
Outdated
| // bit 23-30 unused | ||
| // bit 31 = (flag) unset means SysInfoLogger managed message, set means message managed elsewhere | ||
| // bits 32-63 = sequence number | ||
| return seqN<<32 | (nonSysInfoLogMsg?nonSysInfoLogMsgMask:0) | year<<9 | month<<5 | day; |
There was a problem hiding this comment.
Does this work? seqN is an unsigned seqn<<32 will be 0 (or undefined).
There was a problem hiding this comment.
It works. Should I make all the vars 64bit to make it clearer?
|
Also should the commit message be "sequence" rather than "sequency"? |
|
reflection: Some unit tests would have caught the issue. Are there any unit tests for this set of functionality? |
Use id or msgid for unique SysInfoLogger message identifier Use SeqNum for the sequence number used to the generated ensure id/msgid is unique Signed-off-by: Shamser Ahmed <[email protected]>
shamser
changed the title
Use id or msgid for unique SysInfoLogger message identifier Use SeqNum for the sequence number used to the generated ensure id/msgid is unique
Type of change:
Checklist:
Smoketest:
Testing: