Skip to content

Feature/initial attempt #72

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

Closed
wants to merge 34 commits into from

Conversation

AdamUdovich
Copy link

@AdamUdovich AdamUdovich commented Dec 17, 2018

Diff for rolling compression.

{
gzipStream.Write(buffer, 0, bytesRead);
}
inputStream.CopyTo(gzipStream, bufferSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a small buffer size is going to result in sub-par compression ratios - what's the purpose of overriding the default buffer size here? (I guess the default is 64K?)

var buffer = new byte[1024];
int bytesRead;
int bufferSize = 1024;
var buffer = new byte[bufferSize];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think buffer is being used?

@@ -174,7 +190,8 @@ void AlignCurrentFileTo(DateTime now, bool nextSequence = false)
{
var directoryFileName = Path.GetFileName(directoryFile);

if (!(directoryFileName.Contains("-GZip")))
// Not sure if we can assume log files will always be .txt?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope 😄

The filename (and extension) are configurable by the user

/// </summary>
/// <param name="logFile"> The log file to be compressed </param>
/// <param name="logDirectory"> The directory that holds logFile </param>
public void GZipCompress(string logFile, string logDirectory)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why public?

@nblumhardt
Copy link
Member

@AdamUdovich I think #80 is going to enable this; I'm catching up and doing a bit of housekeeping, so I'll close the PR here - kudos for pushing it forwards :-)

@nblumhardt nblumhardt closed this Apr 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants