Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DAOS-14698 crtl: Create container with attributes #15300
base: master
Are you sure you want to change the base?
DAOS-14698 crtl: Create container with attributes #15300
Changes from 3 commits
5fcd159
315f440
865f2e5
4dd51bd
819b856
b922f70
825512d
2e9afbd
2d26f4d
055c3a0
a3d0a02
7f26733
5e72550
5dca3b4
98bd3c2
a390e8a
9af9320
d2ee639
90186bc
0561a5b
263168f
436436f
f30184d
b1aaa8f
af54e76
d230c87
e3f510d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
im not a fan of this new API. it might be useful to have something for the command line.
but for the API, it's not a big deal to do
daos_cont_create()
daos_cont_set_attr()
I would suggest to remove it.
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 to fully understand your comments.
Are you suggesting to remove
duns_create_path_attr()
andduns_create_path()
functions or just the first one.If we are just removing
duns_create_path_attr()
, then we will have two solutions from my understanding:duns_create_path_attr()
to not create and bind the container in the same function. By this way, we will be able to use the functiondaos_cont_set_attr()
between the two function calls.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.
no you cannot remove duns_create_path()!
my question is why you are adding this new API (duns_crreate_path_attr()) ?
what is the challenge of doing:
duns_create_path()
daos_cont_set_attr()
?
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.
From my understanding, doing this will not work as the attributes are cached inside DFuse after the binding.
Thus, when we will get back from
duns_create_path()
, the call todaos_cont_set_attr()
will not be taken into account by DFuse.@ashleypittman could you confirm this.
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.
I've taken a closer look and think you're both right, having a new API doesn't seem like the right solution here, however I hadn't realised it was the same C function that both created the container and inserted it into the POSIX namespace.
The best path might be:
daos_cont_create()
daos_cont_set_attr()
duns_link_cont()
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.
I was not aware of the duns_link_cont() function, which for sure is a better solution.
Thanks @ashleypittman for the hints and @mchaarawi for the relevant remark :-)
duns_create_path_attr()
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.
Fixed with commit 819b856
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.
Checking - do these tests really need to be
pr
? Or could they bedaily_regression
orfull_regression
?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.
I have no opinion on this, and do not fill legitimate to have one.
I will let you decide with the other test members.
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.
We need your input :) Roughly, it's something like
pr
daily_regression
weekly_regression
IMO these new tests belong in
daily_regression
unless they are very critical to base funcionality. @ashleypittman thoughts?Related, I think we should add the
daos_cmd
feature tag to these tests since they verify some GO code paths for thedaos
commandThere 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.
daos_cmd
feature tagpr
todaily_regression
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.
Fixed with commit 7f26733
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.
Nice. This will be useful for some other tests as well. Once this lands I'll write up a ticket to identify those (for the test team to handle)
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.
Actually it would be an idea to have a way of exporting these from dfuse somehow. For now having a python function in ftest would be useful and then if we have a more direct way of querying it then we can plug it in later.
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.
I meant it's nice to specify
attrs
on create, not necessarily dfuse though.If dfuse already has the attrs what would container create need them for?
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.
Oh, I see what you mean. My mistake.