Skip to content

Lazily create logging files to avoid creating empty files - #775

Open
niqiangpro-cell wants to merge 3 commits into
apache:masterfrom
niqiangpro-cell:lazily-create-log-files
Open

niqiangpro-cell wants to merge 3 commits into
apache:masterfrom
niqiangpro-cell:lazily-create-log-files

Conversation

@niqiangpro-cell

Copy link
Copy Markdown

commit for issue #745

@rm5248

rm5248 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

I like the idea of having this be opt-in - any thoughts @swebb2066 ?

@swebb2066

Copy link
Copy Markdown
Contributor

I like the idea of having this be opt-in - any thoughts

I support the intent of this PR. I don't think empty log files (or directory creation) are useful when they are not used.

@niqiangpro-cell
niqiangpro-cell force-pushed the lazily-create-log-files branch from 572d66d to dd232ee Compare October 12, 2026 09:16
@niqiangpro-cell

Copy link
Copy Markdown
Author

I fixed the compilation error. It looks like the workflows are waiting for maintainer approval. Could you please trigger or approve the CI run?

Comment thread src/main/cpp/fileoutputstream.cpp Outdated
{
int style = APR_FILEPATH_ENCODING_UNKNOWN;
apr_filepath_encoding(&style, pool.getAPRPool());
char* path = nullptr;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This section should be replaced with:
char* path = this->path.getAPRPath();

Comment thread src/main/cpp/fileoutputstream.cpp Outdated
{
m_priv->open(append);
m_priv->logappend = append;
m_priv->path = filename;

@swebb2066 swebb2066 Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

path is assigned in the FileOutputStreamPrivate constructor.

@niqiangpro-cell
niqiangpro-cell force-pushed the lazily-create-log-files branch from dd232ee to 3055feb Compare October 13, 2026 16:05
@swebb2066

swebb2066 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

As it stands, this will break MultiprocessRollingFileAppender because it calls FileOutputStream::getFilePtr after a FileOutputStream is created.

Please also replace MultiprocessRollingFileAppenderPriv::log_file with FileOutputStreamPtr fileOutputStream, and remove all resets of log_file (and associated comments). Then replace the single usage of log_file as follows.

diff --git a/src/main/cpp/multiprocessrollingfileappender.cpp b/src/main/cpp/multiprocessrollingfileappender.cpp
index 0b4bd42e..5ec5c68e 100644
--- a/src/main/cpp/multiprocessrollingfileappender.cpp
+++ b/src/main/cpp/multiprocessrollingfileappender.cpp
@@ -196,7 +196,7 @@ bool MultiprocessRollingFileAppender::isAlreadyRolled(const LogString& fileName,
        if (pSize)
                wantedInfo |= APR_FINFO_SIZE;
        apr_finfo_t finfo1;
-       apr_status_t st1 = apr_file_info_get(&finfo1, wantedInfo, _priv->log_file);
+       apr_status_t st1 = apr_file_info_get(&finfo1, wantedInfo, _priv->fileOutputStream->getFilePtr());

        if (st1 != APR_SUCCESS)
                LogLog::warn(LOG4CXX_STR("apr_file_info_get failed"));

@swebb2066 swebb2066 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a comment in the header file that this class assumes a single active thread.

The single active thread guarantee depends on the mutex lock in AppenderSkeleton::doAppend

Comment thread src/main/cpp/fileoutputstream.cpp Outdated
{
FileOutputStreamPrivate(const LogString& filename)
: path(filename)
: path(filename), logappend(false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a bool append parameter to the constructor

Comment thread src/main/cpp/fileoutputstream.cpp Outdated
apr_file_t* fileptr{ nullptr };
bool logappend;
void open(bool append);
void openAsTruncatedIfAlreadyExists(bool append);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove the bool append parameters and use this->logAppend in the open methods.

@atari83

atari83 commented Oct 14, 2026

Copy link
Copy Markdown

So this laziness behavior for log file creation happens through an optional param/setting right ?
It'd be great to preserve existing mode (thick provisioning) and also have this for devs who want this feature.

Thanks.

@niqiangpro-cell

Copy link
Copy Markdown
Author

So this laziness behavior for log file creation happens through an optional param/setting right ? It'd be great to preserve existing mode (thick provisioning) and also have this for devs who want this feature.

Thanks.

Good point. The current PR changes the behavior unconditionally, but I agree that we should preserve the existing behavior by default and make lazy file creation opt-in. I plan to add an optional setting on FileAppender (and derived file-based appenders), as file creation is really an appender concern. I'd be happy to hear any suggestions you may have.

@niqiangpro-cell
niqiangpro-cell force-pushed the lazily-create-log-files branch from 3055feb to eadb74f Compare October 14, 2026 15:30
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.

5 participants