rzo1 commented on code in PR #8415:
URL: https://github.com/apache/storm/pull/8415#discussion_r2876406365
##########
storm-client/src/jvm/org/apache/storm/metric/FileBasedEventLogger.java:
##########
@@ -91,6 +103,10 @@ public void prepare(Map<String, Object> conf, Map<String,
Object> arguments, Top
String stormId = context.getStormId();
int port = context.getThisWorkerPort();
+ int rotationSizeMb =
ObjectReader.getInt(conf.get(Config.TOPOLOGY_EVENTLOGGER_ROTATION_SIZE_MB),
1000);
Review Comment:
The default (=1gb) might be a bit high. Maybe just go for 100 instead.
##########
storm-client/src/jvm/org/apache/storm/metric/FileBasedEventLogger.java:
##########
@@ -91,6 +103,10 @@ public void prepare(Map<String, Object> conf, Map<String,
Object> arguments, Top
String stormId = context.getStormId();
int port = context.getThisWorkerPort();
+ int rotationSizeMb =
ObjectReader.getInt(conf.get(Config.TOPOLOGY_EVENTLOGGER_ROTATION_SIZE_MB),
1000);
+ this.maxFileSize = rotationSizeMb * 1024L * 1024L;
+ this.maxRetainedFiles =
ObjectReader.getInt(conf.get(Config.TOPOLOGY_EVENTLOGGER_MAX_RETAINED_FILES),
5);
Review Comment:
Please introduce constants for those magic numbers. For example:
```
private static final long BYTES_PER_MB = 1024L * 1024L;
private static final int DEFAULT_ROTATION_MB = 1000;
private static final int DEFAULT_MAX_RETAINED = 5;
```
##########
storm-client/src/jvm/org/apache/storm/metric/FileBasedEventLogger.java:
##########
@@ -46,11 +49,20 @@ public class FileBasedEventLogger implements IEventLogger {
private BufferedWriter eventLogWriter;
private ScheduledExecutorService flushScheduler;
private volatile boolean dirty = false;
+
+ // File rotation configs
+ private long maxFileSize;
+ private int maxRetainedFiles;
+ private long currentFileSize = 0;
private void initLogWriter(Path logFilePath) {
try {
LOG.info("logFilePath {}", logFilePath);
eventLogPath = logFilePath;
+
+ File file = eventLogPath.toFile();
Review Comment:
```
currentFileSize = Files.exists(eventLogPath)
? Files.size(eventLogPath)
: 0L;
```
imho there is no need to create a file object here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
