Allow for primary log rotation by copy and truncate rather than rename #2275

Merged
merged 1 commit into from Apr 24, 2013

4 participants

@othatbrian

I found that Yii rotates files by renaming them and creating a new primary log file when necessary. This poses a problem for any tailing programs which rely on the inode staying constant. I found Paper Trail's remote_syslog (https://github.com/papertrail/remote_syslog) to be one such program.

With this commit, I added the boolean option "rotateByCopy" to CFileLogRoute.php to allow the user to request that the primary log file(s) be rotate by "copy and truncate" rather than "rename".

@resurtm resurtm commented on the diff Mar 30, 2013
framework/logging/CFileLogRoute.php
@@ -46,6 +50,10 @@ class CFileLogRoute extends CLogRoute
* @var string log file name
*/
private $_logFile='application.log';
+ /**
+ * @var boolean rotate by copy and truncate
+ */
+ private $_rotateByCopy=False;
@resurtm
resurtm added a line comment Mar 30, 2013

Falsefalse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@resurtm resurtm commented on the diff Mar 30, 2013
framework/logging/CFileLogRoute.php
@@ -130,6 +138,22 @@ public function setMaxLogFiles($value)
}
/**
+ * @return boolean whether to rotate primary log by copy and truncate
+ */
+ public function getRotateByCopy()
@resurtm
resurtm added a line comment Mar 30, 2013

No need to introduce new virtual attribute with very simple getter and setter methods (getRotateByCopy() and setRotateByCopy()). New public property shall be enough.

UPD: on the other hand it makes sense (class wide consistency is kept—all the other properties are virtual with getters and setters).

@othatbrian
othatbrian added a line comment Mar 30, 2013

My apologies for the getter and setter; I'm a PHP newbie (I'm Perlist/Rubyist), and tried to do my best to follow convention. I was mostly concerned with the implementation, which worked for our integration with Paper Trail's remote_syslog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@klimov-paul klimov-paul commented on the diff Mar 30, 2013
framework/logging/CFileLogRoute.php
@@ -177,6 +201,15 @@ protected function rotateFiles()
}
}
if(is_file($file))
- @rename($file,$file.'.1'); // suppress errors because it's possible multiple processes enter into this section
+ {
+ // suppress errors because it's possible multiple processes enter into this section
+ if($this->getRotateByCopy())
+ {
+ @copy($file,$file.'.1');
+ $fp=@fopen($file,'a') and @ftruncate($fp,0) and @fclose($fp);
+ }
+ else
+ @rename($file,$file.'.1');
@klimov-paul
Yii Software LLC member
klimov-paul added a line comment Mar 30, 2013

If we introduce "copy and truncate" log rotation, do we need to support rename?
May be we should remove this alternative?

@qiangxue
Yii Software LLC member
qiangxue added a line comment Mar 30, 2013

Why not? This only affects some programs. File copy is expensive, especially if you set the log file size to be big.

@klimov-paul
Yii Software LLC member
klimov-paul added a line comment Mar 30, 2013

I see.

@othatbrian
othatbrian added a line comment Mar 30, 2013

Completely agreed; that's why I left the rename, and added the "option" for "copy/truncate".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@othatbrian

My apologies for the getter and setter; I'm a PHP newbie (I'm Perlist/Rubyist), and tried to do my best to follow convention. I was mostly concerned with the implementation, which worked for our integration with Paper Trail's remote_syslog.

@samdark
Yii Software LLC member

No need for apologies, it's a regular process of imporoving a pull request. We're just trying to make code better by providing critics and discussing it.

@othatbrian

Is there any sort of schedule for a "yay/nay" decision on inclusion of this?

@samdark
Yii Software LLC member

@bdstevens no schedule. We're reviewing issues and making decisions when we have enough time to fully review code and understand all possible inplications.

@othatbrian

Cool, thanks! I didn't mean to sound pushy, just curious.

@samdark
Yii Software LLC member

btw., can you address what was commented on the code?

@cebe cebe was assigned Apr 24, 2013
@cebe
Yii Software LLC member

Will handle it.

@cebe cebe added a commit that referenced this pull request Apr 24, 2013
@cebe cebe a bit of refactoring for PR #2275 + CHANGELOG line 3b28868
@cebe cebe merged commit 7b3673b into yiisoft:master Apr 24, 2013
@cebe
Yii Software LLC member

Thanks @bdstevens!

@othatbrian

@cebe, thanks for tweaking my code and accepting it.

@klimov-paul
Yii Software LLC member

@cebe, at 3b288 you have leave setter and getter for "rotateByCopy", while publisize it.

@klimov-paul
Yii Software LLC member

@cebe, also there should be "@since 1.1.14" tag for the "rotateByCopy" field.

@cebe cebe added a commit that referenced this pull request Apr 25, 2013
@cebe cebe Cleanup after 3b288 and PR #2275 c3e647e
@cebe
Yii Software LLC member

Damn, guess it's been too late yesterday... Fixed in c3e647e.
Thanks for reviewing!

@s-larionov s-larionov added a commit to s-larionov/yii that referenced this pull request Apr 26, 2013
@cebe cebe a bit of refactoring for PR #2275 + CHANGELOG line 044123b
@s-larionov s-larionov added a commit to s-larionov/yii that referenced this pull request Apr 26, 2013
@cebe cebe Cleanup after 3b288 and PR #2275 df67c00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment