Skip to content
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

feat: add support for gzip archive files #157

Merged
merged 6 commits into from
Jan 17, 2023

Conversation

mhewedy
Copy link

@mhewedy mhewedy commented Jan 13, 2023

This PR is to support gz files as asked in #132 and #131

@mhewedy
Copy link
Author

mhewedy commented Jan 13, 2023

I think PR is not ready yet ...

as while I am doing a couple of manual tests, I faced this error :

image

However, the file is still there and I can cat it normally.. this happens on Linux and not windows, on windows it works just fine.

Any ideas?

@mhewedy
Copy link
Author

mhewedy commented Jan 13, 2023

I figured out the issue when I checked the WS response (server log was silient though)...

the issue was related to logs path config... I was customized it to look into some specified folder in '/mnt/logs'

So to fix the issue, I added

{
  path: "/tmp/**/*"
}

in my config file as well to logs property.

So it has nothing to do with PR, unless the user should be aware of the paths config in such case!

@sevdokimov
Copy link
Owner

Thanks! Good idea.
But unpacking all log files automatically is not a safe operation, logs may take a lot of disk space. Let's add log-viewer.unpack-archive configuration property and disable it by default.
The ideal solution is ask the user if he want to unpack log because it is not expected. Adding asking of the user is not a trivial fix, I'll implement it later.

@sevdokimov
Copy link
Owner

I think PR is not ready yet ...

as while I am doing a couple of manual tests, I faced this error :

image

However, the file is still there and I can cat it normally.. this happens on Linux and not windows, on windows it works just fine.

Any ideas?

Don't change Log.file property, this property is used for permission checks. You can add Log.realFile property, that should be initialized in Log.LogSnapshot.getChannel() if it's null

@mhewedy
Copy link
Author

mhewedy commented Jan 13, 2023

realFile

I searched the entire code base but didn't find a track for realFile.. can you elaborate more on this?

@mhewedy
Copy link
Author

mhewedy commented Jan 13, 2023

I've updated the PR by suggested comments:

  1. introduce log-viewer.unpack-archive
  2. check the gzip file extension instead

@sevdokimov
Copy link
Owner

realFile

I searched the entire code base but didn't find a track for realFile.. can you elaborate more on this?

I mean you have to add Log.realFile to com.logviewer.data2.Log class. Now com.logviewer.data2.Log has file field, your code replace the value to the temporary file. I propose to keep the original file name in Log.file , but put unpacked file to Log.realFile.
I can add it after merge.

tempFile.deleteOnExit();

try (GZIPInputStream gis = new GZIPInputStream(
Files.newInputStream(file.toFile().toPath()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to convert it to File, and then back to Path? Or is it just mistake?

@@ -141,6 +147,25 @@ public Snapshot createSnapshot() {
}
}

private void decompressAndCopyGZipFile() throws IOException {

File tempFile = File.createTempFile("log-viewer-", "-" + file.getName(file.getNameCount() - 1) + ".tmp");
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to change it to NIO2

Path tempFile = Files.createTempFile("log-viewer-", "-" + file.getName(file.getNameCount() - 1) + ".tmp");
Files.deleteIfExists(tempFile);

Copy link
Author

@mhewedy mhewedy Jan 16, 2023

Choose a reason for hiding this comment

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

I did some searches, and find out there's no direct way in java nio API to get from gz to SeekableByteChannel.
I have to introduce some external dependencies and/or write custom code to accomplish this.

I appreciate your help on this.

@mhewedy mhewedy changed the title feat: add support for tgz archive files feat: add support for gzip archive files Jan 16, 2023
@mhewedy
Copy link
Author

mhewedy commented Jan 16, 2023

BTW, I did other changes as well to suit needs
master...mhewedy:log-viewer:develop

Let me know if any interesting feature that I can contribute back

@sevdokimov sevdokimov merged commit bf489e9 into sevdokimov:master Jan 17, 2023
@sevdokimov
Copy link
Owner

I added the following changes after merge:
1.) renamed the configuration property to log-viewer.unpack-gz-archives
2.) Fixed checking of log visibility: no error if .gz file is visible, but the unpacked '/tmp/log-viewer*.tml' is not.
3.) Added the details error message when the user is trying to open .gz file with disabled log-viewer.unpack-gz-archives
4.) added tests

@mhewedy
Copy link
Author

mhewedy commented Jan 17, 2023

Good, Thanks 🌹

@mhewedy mhewedy deleted the master branch January 19, 2023 16:06
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