Adding rollingFile

[liran2000]

Enhancement of rolling file by rollingFile configuration option.

Rolling file by indexes, based on SizeBasedRotationPolicy, similar to log4j RollingFileAppender (defaults to false).

Algorithm Implementation is taken and edited to fit from log4j RollingFileAppender.

Backward compatible:
All unit tests are passing. Existing options and implementations remain untouched.

rollingFile enhancement is fully covered in a new unit test, which prints more than the max data, then validates that all backup indexes files exists, and do a sample test on the byte content of the files.

[vy]

@liran2000, I have spent quite some time thinking on this issue. I want to share some thoughts your request has triggered and the tentative plan I have in mind.

1. For one, a stateless rolling mechanism grants the assumption that filePattern exhibits a chronological order when files are sorted in lexicographical order. That is, output-%d{yyyyMMdd}.txt works, whereas output-%d{MMddyyyy}.txt doesn't. I think we agree on this. We need to explicitly state this in the manual README.
2. The more I think about whether rolling should be a part of the RotationConfig or not, the more I am convinced that it is just a simple callback logic that is executed once at initialization and after every rotation. Hence, we can simply implement it as a RotationCallback and users can pass it via RotationConfig.Builder#callback(RotationCallback).
   Though this insight gave me another embarrassing enlightenment: The very same applies to compression too! Why the heck did I add RotationConfig.Builder#compress(boolean) rather than simply implementing it as a callback?
   In retrospect, I will implement compression and rolling as callbacks.
3. While reading the sources, I have also noticed that the used scheduler is not closed when RotatingFileOutputStream#close() is called. Since RFOS doesn't create its own scheduler, but receives one, I think it is the responsibility of the call site to shut it down. Yet this might result in partial work once RFOS#close() is called, e.g., compression can still be going on, even though the try-with-resource block is completed. The only way for the user to ensure a complete shutdown is to manually close either his/her own custom scheduler, or the one returned by RotationConfig.getDefaultExecutorService(). This doesn't feel idiomatic to me:

try (RotatingFileOutputStream stream = new RotatingFileOutputStream(config)) {
    // Do great stuff with the "stream"!
}
RotationConfig.getDefaultExecutorService().shutdown();

Hence, I want to come up with an API that users can ensure an ultimate completion:

RotationConfig config = RotationConfig
        .builder()
        .file("/tmp/app.log")
        .filePattern("/tmp/app-%d{yyyyMMdd-HHmmss.SSS}.log")
        .policy(DailyRotationPolicy.instance())
        .build();
try (RotationContext context = RotationContext
        .builder()
        .clock(clock)           // (optional)
        .executor(executor)     // for background tasks, e.g., compression (optional)
        .scheduler(scheduler)   // for time-sensitive background tasks, e.g., rotation, rolling (optional)
        .build();
      RotatingFileOutputStream stream = new RotatingFileOutputStream(config, context)) {
    // Do great stuff with the "stream"!
}   // Will block until all streams and executors/schedulers are closed!

All the aforementioned changes will be backward incompatible, hence I am targeting a 1.0.0 release.
I would really appreciate it if you can share your thoughts about the plan I have shared above.

[liran2000]

Basic idea is to keep it simple, and similar to common logging frameworks.

1. When rolling file is used, file pattern cannot be used, it also does not have same meaning here, so I don’t think it should go together. File names would be as common logging frameworks implementations: like app.log, app.log.1, app.log.2 etc’. It is also enforced in building the object:
   if (filePattern != null) {
   throw new IllegalArgumentException("filePattern cannot be set when using rollingFile.");
   }
   https://github.com/vy/rotating-fos/pull/14/files#diff-89237606108dcf44f0dd9a60c669c0b8efca6ee90b3cf8c102f8ed0226869231R295
   it is also included in a new unit test, rollingFileTest - which can be run and simply see it.
2. For rolling file, not sure a callback would fit. As rolling file implementation is something common to other logging framework, I would expect it to be integrated in the framework implementation like here, and the config would hold just the configuration – whether rolling file should be used or not, and the size based policy.
3. Good point. I think the executorService should be moved from the config to the RotatingFileOutputStream, initialized at its initialization and closed on close() – can be treated as a resource, similar to the underlying stream itself – that way we are consistent, and it is handled automatically for the user similar to other common streams.

Thank you for your outputs, you can share your thoughts on my comments.

[vy]

@liran2000, I have pushed a modified version of your work to maxBackupCount branch. Would you mind checking it, please?

Rather than creating a RotationContext breaking the backward compatibility, I have introduced RotationPolicy#stop() to master in #26.

I have another question: I doubt if rolling can work in combination with compression. Because when compression is enabled, there will be a background task renaming <file>.%i to <file>.%i.gz. Putting aside the complication of checking for both <file>.%i and <file>.%i.gz, while this processing is running, one cannot simply rename <file>.%i files to <file>.%<i+1>. Do you see what I mean? How shall we address this? Disallow rolling when compression is enabled?

[liran2000]

@vy looks good. Some comments:

• backupFile(File[] backupFileRef): Looks a bit less intuitive to expect to pass {null} and return array with a single element. For a solution, either not extract it to a function, or return some new response class for the file and success status (true/false).
  It is not mandatory to change it, your call.
  https://github.com/vy/rotating-fos/compare/maxBackupCount#diff-34ea7c59061c317c356125da7abe79b95f577c0032ba815a771129d40fa59736R175
• As for compress, you are right. Since it will be rolling by size and max backup index anyway, I think it is better to disallow rolling when compression is enabled. If you can add validation on it on the builder.
• As for the unit test, I see you manually triggered rotation. I would prefer to leave my original unit test as well, or something similar, that would auto rotate file based on the size, to validate it is working with normal flow.

Thanks

[vy]

Thanks so much for assisting me in this story @liran2000! I will improve the docs a bit more, publish a release, and close this story.

[vy]

0.9.3. should appear in Maven Central momentarily with maxBackupCount feature.