-
Notifications
You must be signed in to change notification settings - Fork 35
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
xrootd: add stat request #221
Conversation
actually, before implementing also, this will make other tests cleaner, spelling out the correct semantics like: f, err := fs.Open(...)
if err != nil { ... }
defer f.Close() |
Yep, I'll do. |
as for the specs, I agree that the fact that |
xrootd/xrdproto/stat/stat.go
Outdated
// NewRequestForPath forms a Request according to provided path | ||
// and virtualFS parameter which indicates whether virtual file system information | ||
// should be returned instead of default stat information. | ||
func NewRequestForPath(path string, virtualFS bool) *Request { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this function (nor the one below: NewRequestForHandle
) actually improves the usability.
consider:
req := stat.NewRequestForPath("/foo/bar", false)
req := stat.Request{Path: "/foo/bar"}
or:
req := stat.NewRequestForPath("/foo/bar", true)
req := stat.Request{Path: "/foo/bar", Options: stat.OptionsVFS}
IMHO, it reads better using the named-struct-fields way :)
I would have probably gone with the NewXyz
functions for forward compatibility in the event that stat.Request
gained new Options
or new fields (recouped from the [11]byte
reserve), but the specs require that everything is zeroed, so new versions of Request
would have to differ.
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the specs require that everything is zeroed
I got it this way: in the future, these zeroed values can be used for passing some parameters that will have zero as their default value.
Let's assume that some of the reserved values are changed to the meaningful parameter. Probably the default value (0
) will be suitable for the most of cases, so backward compatibility will be kept.
I think that in most of the cases we will not want to change these default values (to keep backward compatibility from our side too). So, NewRequestForPath
will not be modified and another func will be introduced.
From that point of view, it's ok to use Request{ ... }
instead of NewRequestForXyz
.
while still waiting on an answer on xrootd/xrootd#728, could you please rebase and skip the failing test (but also adding a thanks. |
c359774
to
b1d13ba
Compare
Updates go-hep#170.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Note that TestFile_FetchVirtualStatInfo is failing with an error:
xrootd: error 3001: Required argument not present
. I think that it's because it is called withhandle
instead ofpath
(FileSystem
calls it withpath
and it works correctly),Can you take a look at specs for kXR_stat, please?
Especially:
I think that since there is no mention of
handle
it's not obvious thatkXR_stat
requirespath
to be passed and doesn't work when onlyhandle
is present.Am I right? I'm a bit in doubt since I'm not very good at English and
requests need not specify an existing filesystem object
gives a hint thathandle
would not work.I'll fill an issue if you too think that documentation is not clear on that topic.