FtpServer
  1. FtpServer
  2. FTPSERVER-15

FTPServer should use commons-logging and should not output to System.out

    Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:
      log4j

      Description

      I don't want to implement loggers component for each plug-in, that I embedding to my system. All components of Apache should use commons-logging, to simplify embedding or configuring with log4j.

      Not need to use unique Logger component

      Patch will be need soon, after merging source tree with other patches.

      1. patchLoggerPanel.txt
        11 kB
        Sergey Vladimirov
      2. patch.txt
        118 kB
        Sergey Vladimirov
      3. patch.txt
        80 kB
        Sergey Vladimirov

        Activity

        Hide
        Sergey Vladimirov added a comment -

        Feel free to close.

        Discussion about using LogFactory in commands should be continued in mail list

        Show
        Sergey Vladimirov added a comment - Feel free to close. Discussion about using LogFactory in commands should be continued in mail list
        Hide
        Sergey Vladimirov added a comment -

        What about commands?

        I prefer that any command have they own logger (as in CVS), but calling LogFactory on each log write is bad solution in perfomance sence and in model.

        And logger not depends on handler or request.

        This should be another solution

        Show
        Sergey Vladimirov added a comment - What about commands? I prefer that any command have they own logger (as in CVS), but calling LogFactory on each log write is bad solution in perfomance sence and in model. And logger not depends on handler or request. This should be another solution
        Hide
        Rana Bhattacharyya added a comment -

        Oh! I missed one point.

        LogFactory.getLog() internally calls LogFactory.getFactory().getInstance()

        We just broke it down. We call the static LogFactory.getFactory() in FtpConfigImpl class and wraps this by FtpLogFactory to capture all the log messages (proxy log). This LogFactory will be passed to all the classes, where logFactory.getInstance() will be called to Log (proxy) object.

        Show
        Rana Bhattacharyya added a comment - Oh! I missed one point. LogFactory.getLog() internally calls LogFactory.getFactory().getInstance() We just broke it down. We call the static LogFactory.getFactory() in FtpConfigImpl class and wraps this by FtpLogFactory to capture all the log messages (proxy log). This LogFactory will be passed to all the classes, where logFactory.getInstance() will be called to Log (proxy) object.
        Hide
        Rana Bhattacharyya added a comment -

        I have commited the changes due to commons-logging. Few points about it:

        1. lib path changed. Now the structure is:

        <ROOT>

        ---- common
        ---- classes
        ---- lib

        We will store the standard log4j properties file in common/classes directory. We will not bundle in ftpserver jar file so that it will be easier to configure the logging.

        2. All the scripts wil be in bin directory. We should not have two sets of scripts (one for development and one .for binary distribution).

        3. Maven files and scripts have been modified due to directory structure change.

        4. LogFactory.getLog(Class) will not be called in any classes. I somehow do not like to get the Log instance using the static method in all the classes. IMHO it reduces maintainability. Instead LogFactory will be created in FtpConfigImpl (LogFactory.getFactory()) and this LogFactory will be passed using setLogFactory() method. Whenever necessary different classes will use logFactory.getInstance(Class) to get the Log object. As we are not using LogFactory.getLog() static methods in all the classes, we will have the centralized control over LogFactory.

        5. Log4J is optional. Even if we have this dependency in maven, it is not necessary. If we remove log4j jar, other logging mechanism will be used - JDK logging or console logging. So LoggerPanel should not depend on log4j. If user wants to use a non-log4j log factory, LoggerPanel will not work. So to achive this, I have added one FtpLogFactory class. It just wraps the LogFactory we got by calling LogFactory.getFactory() static method in FtpConfigImpl. All the classes will get this FtpLogFactory instance. Whenever we call logFactory.getInstance(Class), FtpLogFactory will return a proxy log which will have default Log implementation and our custom Log implementation as well. Now LoggerPanel implements Log interface. The LoggerPanel sets this into FtpLogFactory.

        Show
        Rana Bhattacharyya added a comment - I have commited the changes due to commons-logging. Few points about it: 1. lib path changed. Now the structure is: <ROOT> ---- common ---- classes ---- lib We will store the standard log4j properties file in common/classes directory. We will not bundle in ftpserver jar file so that it will be easier to configure the logging. 2. All the scripts wil be in bin directory. We should not have two sets of scripts (one for development and one .for binary distribution). 3. Maven files and scripts have been modified due to directory structure change. 4. LogFactory.getLog(Class) will not be called in any classes. I somehow do not like to get the Log instance using the static method in all the classes. IMHO it reduces maintainability. Instead LogFactory will be created in FtpConfigImpl (LogFactory.getFactory()) and this LogFactory will be passed using setLogFactory() method. Whenever necessary different classes will use logFactory.getInstance(Class) to get the Log object. As we are not using LogFactory.getLog() static methods in all the classes, we will have the centralized control over LogFactory. 5. Log4J is optional. Even if we have this dependency in maven, it is not necessary. If we remove log4j jar, other logging mechanism will be used - JDK logging or console logging. So LoggerPanel should not depend on log4j. If user wants to use a non-log4j log factory, LoggerPanel will not work. So to achive this, I have added one FtpLogFactory class. It just wraps the LogFactory we got by calling LogFactory.getFactory() static method in FtpConfigImpl. All the classes will get this FtpLogFactory instance. Whenever we call logFactory.getInstance(Class), FtpLogFactory will return a proxy log which will have default Log implementation and our custom Log implementation as well. Now LoggerPanel implements Log interface. The LoggerPanel sets this into FtpLogFactory.
        Hide
        Rana Bhattacharyya added a comment -

        I prefer commons-logging at this moment. In fact that is the reason I have added logging dependencies in maven. I shall apply the patch.

        Show
        Rana Bhattacharyya added a comment - I prefer commons-logging at this moment. In fact that is the reason I have added logging dependencies in maven. I shall apply the patch.
        Hide
        Sergey Vladimirov added a comment -

        Today is October, 3

        And who is talking about 'getting out of the Incubator'?

        Show
        Sergey Vladimirov added a comment - Today is October, 3 And who is talking about 'getting out of the Incubator'?
        Hide
        Paul Hammant added a comment -

        Rana?

        If you want this goes in - I'll do it

        On sufferance as I stand by my 24th Sept comment.
        I'm not against commons-logging of course, just would prefer some indirection via a Monitor.

        Show
        Paul Hammant added a comment - Rana? If you want this goes in - I'll do it On sufferance as I stand by my 24th Sept comment. I'm not against commons-logging of course, just would prefer some indirection via a Monitor.
        Hide
        Sergey Vladimirov added a comment -

        1. Can anyone of commiters provide responce for patches? I need to go forward with modifiing User interfaces, but I need to wait what to do with this issue - apply or move to trash.

        2. Can anyone close "closed" issues? I have no rights to do this.

        3. I think Apache Tomcat uses Log4j in same way.

        Show
        Sergey Vladimirov added a comment - 1. Can anyone of commiters provide responce for patches? I need to go forward with modifiing User interfaces, but I need to wait what to do with this issue - apply or move to trash. 2. Can anyone close "closed" issues? I have no rights to do this. 3. I think Apache Tomcat uses Log4j in same way.
        Hide
        Sergey Vladimirov added a comment -

        Patched LoggerPanel to restore ability to specify logging level (only for panel).

        Show
        Sergey Vladimirov added a comment - Patched LoggerPanel to restore ability to specify logging level (only for panel).
        Hide
        Sergey Vladimirov added a comment -

        Pre-final version of patch - all links to common logger of ftpserver replaced with local loggers, created with LogFactory.

        Show
        Sergey Vladimirov added a comment - Pre-final version of patch - all links to common logger of ftpserver replaced with local loggers, created with LogFactory.
        Hide
        Sergey Vladimirov added a comment -

        No, I don't need monitor to logging - monitor may be used to count files downloading, active users, etc.

        Using Log log = LogFactory.getLog(this.getClass()) is standart way to create loggers, it will be available in next patch.

        Show
        Sergey Vladimirov added a comment - No, I don't need monitor to logging - monitor may be used to count files downloading, active users, etc. Using Log log = LogFactory.getLog(this.getClass()) is standart way to create loggers, it will be available in next patch.
        Show
        Paul Hammant added a comment - If you're trying to make a reusable compoent, consider a no-fixed-logging-choice. Refer ... http://www.jadetower.org/muses/archives/000053.html http://wiki.apache.org/avalon/AvalonNoLogging And ... http://svn.picocontainer.codehaus.org/java/picocontainer/trunk/container/src/java/org/picocontainer/ComponentMonitor.java?rev=2419&view=auto And its implementations ... http://svn.picocontainer.codehaus.org/java/picocontainer/trunk/container/src/java/org/picocontainer/monitors/ http://svn.picocontainer.codehaus.org/java/picocontainer/trunk/gems/src/java/org/picocontainer/gems/monitors/ (as well as many other OSS tools that fit this design).
        Hide
        Sergey Vladimirov added a comment -

        All components still use one logger, but connons logger.

        One class (LoggerPanel) uses log4j. Need change to show error, if no log4j

        need to include log4j and commons-logging.jar

        Show
        Sergey Vladimirov added a comment - All components still use one logger, but connons logger. One class (LoggerPanel) uses log4j. Need change to show error, if no log4j need to include log4j and commons-logging.jar

          People

          • Assignee:
            Unassigned
            Reporter:
            Sergey Vladimirov
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development