Uploaded image for project: 'Mesos'
  1. Mesos
  2. MESOS-1104

Move linux/fs.hpp out of `mesos` namespace in linux/fs.h

Details

    • Improvement
    • Status: Accepted
    • Major
    • Resolution: Unresolved
    • None
    • None
    • None

    Attachments

      Activity

        archanakumari Archana kumari added a comment -

        Can anyone advice me a little on fixing this issue...

        archanakumari Archana kumari added a comment - Can anyone advice me a little on fixing this issue...
        adam-mesos Adam B added a comment -

        This JIRA refers to this TODO from benjaminhindman: https://github.com/apache/mesos/blob/0.22.1/src/linux/cgroups.cpp#L64
        Furthermore, idownes suggests that "These three variations on mount information should be consolidated and moved to stout, along with mount and umount.": https://github.com/apache/mesos/blob/master/src/linux/fs.hpp#L40
        Perhaps one of them would like to Accept this issue and volunteer as Shepherd?

        It would also be great if we turned the pseudo-doxygen formatting into proper doxygen/javadoc style.

        adam-mesos Adam B added a comment - This JIRA refers to this TODO from benjaminhindman : https://github.com/apache/mesos/blob/0.22.1/src/linux/cgroups.cpp#L64 Furthermore, idownes suggests that "These three variations on mount information should be consolidated and moved to stout, along with mount and umount.": https://github.com/apache/mesos/blob/master/src/linux/fs.hpp#L40 Perhaps one of them would like to Accept this issue and volunteer as Shepherd? It would also be great if we turned the pseudo-doxygen formatting into proper doxygen/javadoc style.

        Can you please be more elaborate on this issue.?There is no file called linux/fs.h in mesos 0.26 version. Is the issue resolved?

        a10gupta Abhishek Dasgupta added a comment - Can you please be more elaborate on this issue.?There is no file called linux/fs.h in mesos 0.26 version. Is the issue resolved?
        xds2000 Deshi Xiao added a comment -

        dont' know howto start. anyone can do me a favor?

        xds2000 Deshi Xiao added a comment - dont' know howto start. anyone can do me a favor?
        haosdent@gmail.com haosdent added a comment - - edited

        xds2000 I think just move the code in ./src/linux/fs.hpp and ./src/linux/cpp to ./3rdparty/libprocess/3rdparty/stout/include/stout/fs.hpp. Note that we support both windows and posix in stout/fs.hpp. And all functions related to ./src/linux/fs.hpp are only available in Linux. Maybe add something like stout/linux/fs.hpp or add them to stout/posix/fs.hpp

        haosdent@gmail.com haosdent added a comment - - edited xds2000 I think just move the code in ./src/linux/fs.hpp and ./src/linux/cpp to ./3rdparty/libprocess/3rdparty/stout/include/stout/fs.hpp . Note that we support both windows and posix in stout/fs.hpp . And all functions related to ./src/linux/fs.hpp are only available in Linux. Maybe add something like stout/linux/fs.hpp or add them to stout/posix/fs.hpp
        haosdent@gmail.com haosdent added a comment -

        After look other exists code under src/linux/, e.g. perf.hpp, ns.hpp I think remove mesos::internal:: should be enough.

        haosdent@gmail.com haosdent added a comment - After look other exists code under src/linux/ , e.g. perf.hpp , ns.hpp I think remove mesos::internal:: should be enough.
        xds2000 Deshi Xiao added a comment -

        hi haosdent,

        could you please shepherd this issue?

        xds2000 Deshi Xiao added a comment - hi haosdent, could you please shepherd this issue?
        xds2000 Deshi Xiao added a comment - patch: https://reviews.apache.org/r/45500/
        haosdent@gmail.com haosdent added a comment -

        xds2000 I add my basic review comments on it now. For further review, you could send email to dev mailing list and find a committer shepherd you in this ticket.

        haosdent@gmail.com haosdent added a comment - xds2000 I add my basic review comments on it now. For further review, you could send email to dev mailing list and find a committer shepherd you in this ticket.
        xds2000 Deshi Xiao added a comment -

        thanks a lot.

        xds2000 Deshi Xiao added a comment - thanks a lot.
        xds2000 Deshi Xiao added a comment - - edited

        have no linux box to testing on it. please hold on on patched code update.

        xds2000 Deshi Xiao added a comment - - edited have no linux box to testing on it. please hold on on patched code update.
        xds2000 Deshi Xiao added a comment -

        patch updated, anyone can testing it?

        https://reviews.apache.org/r/45500/

        xds2000 Deshi Xiao added a comment - patch updated, anyone can testing it? https://reviews.apache.org/r/45500/
        mcypark Michael Park added a comment -

        xds2000 Could you add some context and goal in the description section? The title also doesn't seem to make sense.

        Beyond that, what are we trying to accomplish here? Does this ticket allow something that we couldn't do before? Are we trying to make these filesystem utilities public...? Based on adam-mesos said above, are we trying to consolidate something into stout?

        I'd be up for shepherding this, but I don't understand what we're trying to do.

        mcypark Michael Park added a comment - xds2000 Could you add some context and goal in the description section? The title also doesn't seem to make sense. Beyond that, what are we trying to accomplish here? Does this ticket allow something that we couldn't do before? Are we trying to make these filesystem utilities public...? Based on adam-mesos said above, are we trying to consolidate something into stout? I'd be up for shepherding this, but I don't understand what we're trying to do.
        xds2000 Deshi Xiao added a comment -

        Michael Hi,

        Thanks for your guide, i have update based on above comments recap. i have update the description. I think the changed is reason with consolidated with other file's convention.

        xds2000 Deshi Xiao added a comment - Michael Hi, Thanks for your guide, i have update based on above comments recap. i have update the description. I think the changed is reason with consolidated with other file's convention.
        xds2000 Deshi Xiao added a comment - https://reviews.apache.org/r/45500/ update again.
        xds2000 Deshi Xiao added a comment - mcypark what's your feedback. https://reviews.apache.org/r/45500/
        xds2000 Deshi Xiao added a comment -

        haosdent mcypark could you please reive it again.https://reviews.apache.org/r/45500/

        xds2000 Deshi Xiao added a comment - haosdent mcypark could you please reive it again. https://reviews.apache.org/r/45500/
        haosdent@gmail.com haosdent added a comment -

        Thank you very much for your patch, it looks to me basically besides I am not sure whether keep it in current folder or move it to stout.

        haosdent@gmail.com haosdent added a comment - Thank you very much for your patch, it looks to me basically besides I am not sure whether keep it in current folder or move it to stout .
        xds2000 Deshi Xiao added a comment -

        i think the file convention is follow the current folder. if move it to stout, also need consider what benefit on the action. Could you please list some concerns let me understand your think?

        xds2000 Deshi Xiao added a comment - i think the file convention is follow the current folder. if move it to stout, also need consider what benefit on the action. Could you please list some concerns let me understand your think?
        haosdent@gmail.com haosdent added a comment -

        xds2000 I think the reason to keep it in current folder is those functions now only can be used in Linux, just like src/linux/ns.hpp. But if we want to support them in osx or windows in the future, I think move to stout would be better because it places the generic utilities.

        haosdent@gmail.com haosdent added a comment - xds2000 I think the reason to keep it in current folder is those functions now only can be used in Linux, just like src/linux/ns.hpp . But if we want to support them in osx or windows in the future, I think move to stout would be better because it places the generic utilities.
        xds2000 Deshi Xiao added a comment -

        sure. i think this is another purpose to handle not this issue's concerns. Maybe the Shepherd can guide it. let it go.

        xds2000 Deshi Xiao added a comment - sure. i think this is another purpose to handle not this issue's concerns. Maybe the Shepherd can guide it. let it go.
        mcypark Michael Park added a comment -

        xds2000 It looks like this patch is simply moving everything in mesos::internal::fs to fs. That is, it doesn't look like we're consolidating multiple implementations. Again, what are we trying to accomplish here? Is there some place where we want access to these functions but for some reason, are not accessible or something?

        mcypark Michael Park added a comment - xds2000 It looks like this patch is simply moving everything in mesos::internal::fs to fs . That is, it doesn't look like we're consolidating multiple implementations. Again, what are we trying to accomplish here? Is there some place where we want access to these functions but for some reason, are not accessible or something?
        xds2000 Deshi Xiao added a comment -

        Thanks mcypark, i guess the mesos::internal prefix namespace is not suitable for fs. this patch is based on above comments. the original TODO's information is very confuse on me.

        ping adam-mesos idownes benjaminhindman could you please give some comments?

        xds2000 Deshi Xiao added a comment - Thanks mcypark , i guess the mesos::internal prefix namespace is not suitable for fs. this patch is based on above comments. the original TODO's information is very confuse on me. ping adam-mesos idownes benjaminhindman could you please give some comments?
        xds2000 Deshi Xiao added a comment -

        hi, anyone can guide the reason to moving the linux/fs.hpp? i actually don't know what reason, just fee this is newbie issue, and follow the comments to do it. but mcypark mentioned above, we need a reasonable demand to answer the quesiton first. if not, please remove the ACCEPT status thanks.

        xds2000 Deshi Xiao added a comment - hi, anyone can guide the reason to moving the linux/fs.hpp? i actually don't know what reason, just fee this is newbie issue, and follow the comments to do it. but mcypark mentioned above, we need a reasonable demand to answer the quesiton first. if not, please remove the ACCEPT status thanks.
        greggomann Greg Mann added a comment -

        xds2000, sorry for the confusion! And thanks for your patch

        I think that this issue still makes sense - mcypark, I don't think the intention was to consolidate implementations. Rather, as Deshi suggested, I think the TODO indicates that a namespace nested within mesos:: doesn't make sense for a header which provides generic Linux functionality that isn't Mesos-specific. Looking at the namespaces declared within src/linux/, it's not entirely consistent, but in most cases we do not use the mesos:: prefix.

        Deshi, if you want to pick this back up, we could try to find a shepherd for the ticket and continue. If you're busy with other things and don't have time, would you mind discarding the review request? We can always reopen it in the future.

        greggomann Greg Mann added a comment - xds2000 , sorry for the confusion! And thanks for your patch I think that this issue still makes sense - mcypark , I don't think the intention was to consolidate implementations. Rather, as Deshi suggested, I think the TODO indicates that a namespace nested within mesos:: doesn't make sense for a header which provides generic Linux functionality that isn't Mesos-specific. Looking at the namespaces declared within src/linux/ , it's not entirely consistent, but in most cases we do not use the mesos:: prefix. Deshi, if you want to pick this back up, we could try to find a shepherd for the ticket and continue. If you're busy with other things and don't have time, would you mind discarding the review request? We can always reopen it in the future.
        xds2000 Deshi Xiao added a comment -

        Thanks for your hints, done.

        xds2000 Deshi Xiao added a comment - Thanks for your hints, done.

        People

          Unassigned Unassigned
          archanakumari Archana kumari
          Votes:
          0 Vote for this issue
          Watchers:
          6 Start watching this issue

          Dates

            Created:
            Updated: