Preparations for https://github.com/serilog/serilog-sinks-rollingfile…

[hmvs]

It's preparation for serilog/serilog-sinks-rollingfile#33

Is there any reason why PeriodicFlushToDiskSink uses ILogEventSink as a parameter for the constructor? I propose to not do downcasting. Do we have some file-based sync that can't be flushed in one way or another?
I believe we should make an interface IFileSink that will be inherited from both ISizeLimitedFileSink, IFlushableFileSink.

Will do it in next commit if you agree.
And should i bump version in project.json?

[nblumhardt]

Hi Vadym, thanks for kicking it off!

I'm in general agreement about downcasting, but there's a trade-off in this instance. Adding small interfaces can be done without breaking changes, whereas once we've created an IFileSink, any future additions, like the property we're adding here, will end up breaking downstream implementers. Maybe not a big deal, as we don't expect there to be many of them, but that was the original reasoning.

(The first version of PeriodicFlushToDiskSink was generic on T: ILogEventSink, IFlushableFileSink and took a T constructor parameter, so there was no need to down-cast, but this led to some annoying usage characteristics so the downcasting version was adopted as the lesser of the two evils :-) )

So, I think while I'm okay with IFileSink, I'd also like to avoid more churning around this design point. Happy to go with IFileSink if it helps avoid issues in the rolling file code, but I don't think we should make the change just to avoid downcasting on principle. Let me know what you think.

I can take care of project.json, no need to worry about that bit 👍

[hmvs]

Yep, we should find a compromise between what interfaces/methods should be included in IFileSink if we create it.
Need to find-out is there any File based sinks which doesn't have Flush() and SizeLimitReached? Can we include this two into IFileSink.
If i do it, what packages potentially i can break?

[nblumhardt]

Thanks for the follow-up. Just looping back,

Happy to go with IFileSink if it helps avoid issues in the rolling file code, but I don't think we should make the change just to avoid downcasting on principle.

Do you think we'll avoid any issues other than downcasting? If not, the multiple-small-interface approach is probably preferable, given the nicer evolutionary path it enables.

[hmvs]

We will avoid a lot of if else statements in all downstream packages. So if you prefer small interfaces you can just merge this commit, so i can proceed with implementation in rollingfile. :) Thanks

[nblumhardt]

Thanks for the follow-up. The more I think about this, the more I feel like the existence of multiple implementations should be wrapped up in the file sink and not a concern for consuming code.

What do you think about turning FileSink into a shell, with all of the interesting methods delegated to internal implementation classes, which the current FileSink and SharedFileSinks would effectively become?

• FileSink would get a new constructor accepting bool shared
• SharedFileSink and IFlushableFileSink would be obsoleted
• SizeLimitReached and so-on would go directly on FileSink

Unlike the interface option, future methods/properties could be added to a FileSink class without breakage.

It would be more work than adding a new interface, but might set us up better in the long run... Impressions?

[hmvs]

Yep, it's fine for me also. Should i do it? Or you?

[nblumhardt]

@hmvs I'm fine either way - it would be great if you have the time to give it a shot. (Sorry about the slow turnaround, really needed to think about this one!)

[hmvs]

I just thought to implement it, but found that now we have two different SharedFileSinks and they are quite different. And if we merge them both to our FileSink it will be quite big and a lot of #if statements. And seems we do not have UT for specific cases how to verify that ATOMIC_APPEND and OS_MUTEX are not broken.

[nblumhardt]

Vadym, thanks for your work on this! Even though it's not possible to merge this directly now, working through it with you greatly clarified the direction we needed to take. I hope the new package structure lowers the friction of contributing in future - if you see more improvements that can be made after #35 please do jump in :-) All the best!