Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal to make the XRootD f-stream future proof. #1432

Closed
abh3 opened this issue Mar 19, 2021 · 7 comments
Closed

Proposal to make the XRootD f-stream future proof. #1432

abh3 opened this issue Mar 19, 2021 · 7 comments
Assignees

Comments

@abh3
Copy link
Member

abh3 commented Mar 19, 2021

Currently, the f-stream contains binary information that is fixed in size which makes it impossible to add additional fields without disrupting collectors. This proposal attempts to correct this problem. Currently, documented as:

https://xrootd.slac.stanford.edu/doc/dev51/xrd_monitoring.htm#_Toc49119289

Specifically, the XrdXrootdMonFileCLS structure is unchangeable. This proposal would add a length header to each segment of XrdXrootdMonStatXFR, XrdXrootdMonStatOPS, and XrdXrootdMonStatSSQ. The additional members that would be added to front of each each structure would be:

short segLen; // Length of this structure
char segrsvd[6]; // Reserved for future use

Note that segLen restricts the structure to be generally less than 32K-1 which is sufficient since the length of the full record is indicated by XrdXrootdMonFileHdr::recSize which is signed as well.

To indicate that the new format is being used, a new record type would be defined. Currently, this record is tagged as isClose in XrdXrootdMonFileHdr::recType. The new version would have the designation isClose2 so the collector could distinguish which version it is dealing with.

Questions:

  1. Should XrdXrootdMonFileHdr::recSize be made unsigned as well as all sub lengths? This would be a more disruptive change should the length of this structure exceed 32K-1 (unlikely to happen any time soon but still). Note that XrdXrootdMonHeader::plen which indicates the full length of the packet which contains these sub-structures is unsigned. Leaving things as they are will unlikely create a problem in the future but it is not technically future-proof.
  2. Should there be an option to generate V1 or V2 records? If so, what should be the default?
@abh3 abh3 self-assigned this Mar 19, 2021
@osschar
Copy link
Contributor

osschar commented Mar 22, 2021

Is the only thing you're after to be able to add new data members/structs for the f-stream close record?

Note that XrdXrootdMonFileCLS is really not very relevant ... one needs to start with XrdXrootdMonFileHdr and then cast it to whatever the recType tells them.

So you are free to add new recTypes, one could be ClosePlusPlus ... and this would then include an additional struct that could have it's own size and version info, or be a json string or I don't know what.

@abh3
Copy link
Member Author

abh3 commented Mar 22, 2021 via email

@osschar
Copy link
Contributor

osschar commented Mar 22, 2021

XFR records can also be sent out periodically to do "status update" on a set of files. So I don't think we can easily change this, unless you also add isXfr2.

If all you add is short segLen to various structs, what is the collector supposed to do with it? Look it up into some table and divine the version and, from it, the contents of the struct? You could use a char out of segrcvd for a version number.

I though adding a new struct where we do all this nastiness and leave what we have alone is actually cleaner.

@abh3
Copy link
Member Author

abh3 commented Mar 23, 2021

Well, actually we would be adding a new record type as original noted (i.e. isclose2). We don't need to add a new isXFR as that record type is easily expandable because the header already has the length of the packet and there is only one struct in that packet. The issue comes about because the isClose record can have up to three structures in it and none of those are self-describing. So, the length of the packet in the header doesn't tell you what the actual size is of any struct in it making them fixed size. What I want to do is add individual lengths to these structs.

Of course, we are ignoring the issue that once we switch to using isClose2 then for collectors that haven't been upgraded it will appear that there are no close records at all.

@djw8605
Copy link
Contributor

djw8605 commented Mar 23, 2021

I would also prefer a new record type. Adding lengths to these fields seems like it would make the collector more complicated. The XrdXrootdMonFileHdr::recFval is already there to specify what type of records are in the CLS. Add new values there, like Ops2...?

@abh3
Copy link
Member Author

abh3 commented Mar 23, 2021

Well, that is the problem. We can't add new values there because the size of the structures there are fixed. The XrdXrootdMonFileHdr::recFval value gives you the total length of the record but there is no way to determine the length of any individual structure in that packet. That is why I proposed to make the structures self-describing by adding the length of the structure to the the structure. So, as new fields are added the length increases but old collectors use the preexisting definition but are able to skip to the next structure by using the structures length. Sure, I can define a new record type. The question is whether is supplants the existing record type or adds yet another record to the whole thing. The latter makes it even more complicated than moving the whole layout to be self-describing.

@abh3
Copy link
Member Author

abh3 commented Dec 13, 2021

Discussion closed, because of no interest.

@abh3 abh3 closed this as completed Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants