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

Feature: Add new header / first line #6

Closed
pitschr opened this issue Nov 24, 2019 · 7 comments
Closed

Feature: Add new header / first line #6

pitschr opened this issue Nov 24, 2019 · 7 comments

Comments

@pitschr
Copy link

pitschr commented Nov 24, 2019

Hello.

Does your library offer the possibility to add a header line (or first line) after e.g. daily rotation?

In my case I am using your library for auditing which offers few formats, including CSV format. As you know CSV are not really human-friendly as it contains only comma-separated values it would be very beneficial to add header automatically when a file rotation happens.

E.g.

headerA,headerB,headerC
valueA1,valueB1,valueC1
valueA2,valueB2,valueC2
... lines omitted
valueAn,valueBn,valueCn

after daily file rotation the header line should be re-added. Actually I see only the possibility to add header when I start writing the file, but doesn't re-apply when daily rotation happens.

If not, I could imagine to add this functionality either as part of RotationConfig or RotationCallback. And the logic if the header line should be added or not may happen in method com.vlkan.rfos.RotatingFileOutputStream#unsafeRotate

Let me know if you are interested in adding this feature or do you prefer a PR?

@vy
Copy link
Owner

vy commented Nov 25, 2019

I would rather prefer this to be implemented by user as a custom RotationCallback acting on #onSuccess(). There you receive the file name, the rest is a matter of echo "header" && cat $filename > $new_filename. That said, I see your use case and providing an alternative #onTrigger() with a filename parameter might come handy. I am off in the upcoming two weeks and I will see what I can do without introducing a domain-specific functionality.

@pitschr
Copy link
Author

pitschr commented Nov 25, 2019

Yes, using onSuccess was my first thought, but then I noticed that onSuccess is not in the synchronization block which may cause some issues. Maybe I'm wrong?

Sure no worries, it is not urgent. If you prefer a PR then please let me know it. It should not be (too) much effort for me.

@vy
Copy link
Owner

vy commented Nov 26, 2019

@pitschr, see the PR #7. I presume it addresses your concern, right? Further, I am planning to replace Timer and Thread usage with ScheduledExecutorService. I don't intend to keep the backward compatibility. Is that okay with you?

@vy
Copy link
Owner

vy commented Nov 29, 2019

Hey @pitschr, will you be able to spare some time to check the changes? Or shall I just put a ribbon around it and make a new release?

@pitschr
Copy link
Author

pitschr commented Nov 30, 2019

Hey.

Sure, as long the library is not used by thousand of developers I won't suggest to take the backward-compatibility seriously and when considering the semantic versioning, then your library has not reached the public release (1.x) anyway. In nutshell, go ahead with changes you want to do.

As the onSuccess() method is now in the synchronized block I can create a custom RotationCallback as initially suggested by you.

I'll check out it in next days. Thank you for your support.

@pitschr
Copy link
Author

pitschr commented Dec 2, 2019

I made quick tests and they are working on Windows machine as well. Appending headers was not a problem anymore. For the test I used

                @Override
                public void onOpen(RotationPolicy policy, Instant instant, OutputStream stream) {
                    try {
                        stream.write("HEADER 1 HEADER 2 ä ö ü ж з к л ✪ ♫".getBytes(StandardCharsets.UTF_8));
                        stream.write(System.lineSeparator().getBytes());
                    } catch (IOException ex) {
                       // do stuff here ...
                    }
                }

Few proposals:

Only one issue I encountered was that header might be added twice times when APPEND mode was used. Therefore it may be highlighted as a comment in JavaDoc so that developers have to think about this scenario as well.

Meaning, they either have to provide two RotationCallbacks implementations if the append is subject to be dynamic or add an own logic by checking if the file is empty before adding headers (check if file is empty -> add header, otherwise not).

If you want to make it more comfortable, then perhaps add ByteCountingOutputStream instead of OutputStream so that they can use stream.size() to distinguish if the file is empty or not. Otherwise they may have to cast to FileOutputStream first to get the file size, using like: ((FileOutputStream)stream).getChannel().size(). Actually the RotationCallback#onOpen(..) method is called by RotatingFileOutputStream only. Do I miss something?`

From my side you can go ahead with releasing the version 0.9 as I am not using the "append" feature at the moment.

@vy
Copy link
Owner

vy commented Dec 3, 2019

Thanks for the great feedback and review @pitschr, I really appreciated that. I prefer not to expose the internals (i.e. ByteCountingOutputStream) to the user. Given I have provided enough control knobs (both onOpen() and onClose()) to the user, I think it is up to the user to decide to what to do next.

I have released 0.9.0 in Sonatype, it should show up momentarily in Maven Central.

@vy vy closed this as completed Dec 3, 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

No branches or pull requests

2 participants