This is an automated email from the ASF dual-hosted git repository.
stefanegli pushed a commit to branch master
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-event.git
The following commit(s) were added to refs/heads/master by this push:
new dae8b46 SLING-11793 : limit log messages count for
JobExecutionContext (#24)
dae8b46 is described below
commit dae8b46d2ea1d4cbfaeaf75e665b62d86a924abb
Author: Rishabh Kumar <[email protected]>
AuthorDate: Wed Mar 15 16:39:21 2023 +0530
SLING-11793 : limit log messages count for JobExecutionContext (#24)
* SLING-11793 : limit log messages count for JobExecutionContext
* SLING-11793 : fixed OOM if we initialized ArrayDeque with
Integer.MAX_VALUE
* SLING-11793 : fixed cases where max progressLog count could be 0 or lower
than default size of ArrayDeque
* SLING-11793 : ensured backward compatability while setting/getting
progressLog property on Job
* SLING-11793 : fixed issue when old job doesn't have progressLog max count
property
* SLING-11793 : removed slingevent:progresslog:maxCount as JCR property and
used as an instance variable
* SLING-11793 : fixed issue when migrating from old job to new job with max
count limit
* SLING-11793 : fixed compatibility issue by keeping the PROGRESS_LOG
property as String[]
* SLING-11793 : removed wiring of progressLogMaxCount and passed this
variable as method parameter to JobImpl
* SLING-11793 : removed wiring of progressLogMaxCount
* SLING-11793 : appended truncated log message in case we had to trim
progressLog message due to maxLog Count
* SLING-11793 : improved documentation of progresslog_maxCount config to
add that using 0 would discard all the logs
* SLING-11793 : returned the tests to their default behaviour of having
unlimited log messages
* SLING-11793 : skipped adding truncated log message if already there in log
---------
Co-authored-by: Rishabh Kumar <[email protected]>
---
.../apache/sling/event/impl/jobs/JobHandler.java | 12 ++-
.../org/apache/sling/event/impl/jobs/JobImpl.java | 34 +++++--
.../impl/jobs/config/JobManagerConfiguration.java | 22 +++++
.../impl/jobs/queues/JobExecutionContextImpl.java | 3 +-
.../apache/sling/event/impl/jobs/JobsImplTest.java | 100 ++++++++++++++++++++-
.../config/JobManagerConfigurationTestFactory.java | 5 +-
6 files changed, 167 insertions(+), 9 deletions(-)
diff --git a/src/main/java/org/apache/sling/event/impl/jobs/JobHandler.java
b/src/main/java/org/apache/sling/event/impl/jobs/JobHandler.java
index 1a0ae86..12e2221 100644
--- a/src/main/java/org/apache/sling/event/impl/jobs/JobHandler.java
+++ b/src/main/java/org/apache/sling/event/impl/jobs/JobHandler.java
@@ -51,6 +51,11 @@ public class JobHandler {
private final JobManagerConfiguration configuration;
+ /**
+ * Max number of log message that can stored by consumer to add
information about current state of Job.
+ */
+ private final int progressLogMaxCount;
+
private final JobExecutor consumer;
public JobHandler(final JobImpl job,
@@ -59,12 +64,17 @@ public class JobHandler {
this.job = job;
this.consumer = consumer;
this.configuration = configuration;
+ this.progressLogMaxCount = configuration.getProgressLogMaxCount();
}
public JobImpl getJob() {
return this.job;
}
+ public int getProgressLogMaxCount() {
+ return progressLogMaxCount;
+ }
+
public JobExecutor getConsumer() {
return this.consumer;
}
@@ -281,5 +291,5 @@ public class JobHandler {
}
}
-
+
}
\ No newline at end of file
diff --git a/src/main/java/org/apache/sling/event/impl/jobs/JobImpl.java
b/src/main/java/org/apache/sling/event/impl/jobs/JobImpl.java
index ab4ba1b..f5956f7 100644
--- a/src/main/java/org/apache/sling/event/impl/jobs/JobImpl.java
+++ b/src/main/java/org/apache/sling/event/impl/jobs/JobImpl.java
@@ -62,6 +62,8 @@ public class JobImpl implements Job, Comparable<JobImpl> {
*/
public static final String PROPERTY_FINISHED_STATE =
"slingevent:finishedState";
+ static final String TRUNCATED_LOG = "...truncated log";
+
private final ValueMap properties;
private final String topic;
@@ -310,16 +312,19 @@ public class JobImpl implements Job, Comparable<JobImpl> {
return Job.PROPERTY_JOB_PROGRESS_ETA;
}
- public String log(final String message, final Object... args) {
+ public String log(final int logMaxCount, final String message, final
Object... args) {
+
+ if (logMaxCount <= 0) {
+ this.properties.remove(Job.PROPERTY_JOB_PROGRESS_LOG);
+ return Job.PROPERTY_JOB_PROGRESS_LOG;
+ }
+
final String logEntry = MessageFormat.format(message, args);
final String[] entries =
this.getProperty(Job.PROPERTY_JOB_PROGRESS_LOG, String[].class);
if ( entries == null ) {
this.setProperty(Job.PROPERTY_JOB_PROGRESS_LOG, new String[]
{logEntry});
} else {
- final String[] newEntries = new String[entries.length + 1];
- System.arraycopy(entries, 0, newEntries, 0, entries.length);
- newEntries[entries.length] = logEntry;
- this.setProperty(Job.PROPERTY_JOB_PROGRESS_LOG, newEntries);
+ addLog(logEntry, entries, logMaxCount);
}
return Job.PROPERTY_JOB_PROGRESS_LOG;
}
@@ -420,4 +425,23 @@ public class JobImpl implements Job, Comparable<JobImpl> {
return "JobImpl [properties=" + properties + ", topic=" + topic
+ ", path=" + path + ", jobId=" + jobId + "]";
}
+
+ // helper methods
+ private void addLog(final String logEntry, final String[] entries, final
int maxSize) {
+
+ final String[] newEntries;
+ if (entries.length >= maxSize) {
+ newEntries = new String[maxSize];
+ System.arraycopy(entries, entries.length - maxSize + 1,
newEntries, 0, maxSize - 2);
+ final String prevLastLog = entries[entries.length - 1];
+ newEntries[maxSize - 2] = prevLastLog.endsWith(TRUNCATED_LOG) ?
prevLastLog.replace(TRUNCATED_LOG, "") : prevLastLog;
+ newEntries[maxSize - 1] = logEntry.endsWith(TRUNCATED_LOG) ?
logEntry : logEntry + TRUNCATED_LOG;
+ } else {
+ newEntries = new String[entries.length + 1];
+ System.arraycopy(entries, 0, newEntries, 0, entries.length);
+ newEntries[entries.length] = logEntry;
+
+ }
+ this.setProperty(Job.PROPERTY_JOB_PROGRESS_LOG, newEntries);
+ }
}
diff --git
a/src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java
b/src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java
index 1c267f6..a66d626 100644
---
a/src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java
+++
b/src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java
@@ -92,6 +92,12 @@ public class JobManagerConfiguration {
description = "Specify the periodic interval in minutes (default
is 48h - use 0 to disable) after which " +
"removed jobs (ERROR or DROPPED) should be cleaned from
the repository.")
int cleanup_period() default 2880;
+
+ @AttributeDefinition(name = "Progress Log message's max count",
+ description = "Max number of log messages that can stored by
consumer to add information about current state of Job.\n" +
+ "Any attempt to add more information would result into
purging of the least recent messages." +
+ "Use 0 to discard all the logs. default is -1 (to
indicate infinite). ")
+ int progresslog_maxCount() default -1;
}
/** Logger. */
private final Logger logger =
LoggerFactory.getLogger("org.apache.sling.event.impl.jobs");
@@ -150,6 +156,11 @@ public class JobManagerConfiguration {
private volatile long startupDelay;
+ /**
+ * The max count of job progress log messages
+ */
+ private int progressLogMaxCount;
+
private volatile InitDelayingTopologyEventListener startupDelayListener;
private volatile boolean disabledDistribution;
@@ -258,6 +269,13 @@ public class JobManagerConfiguration {
// an immediate effect - it will only have an effect on next
activation.
// (as 'startup delay runnable' is already scheduled in activate)
this.startupDelay = config.startup_delay();
+
+ if (config.progresslog_maxCount() < 0) {
+ this.progressLogMaxCount = Integer.MAX_VALUE;
+ } else {
+ this.progressLogMaxCount = config.progresslog_maxCount();
+ }
+
}
/**
@@ -413,6 +431,10 @@ public class JobManagerConfiguration {
return this.jobsBasePathWithSlash;
}
+ public int getProgressLogMaxCount() {
+ return this.progressLogMaxCount;
+ }
+
public String getPreviousVersionAnonPath() {
return this.previousVersionAnonPath;
}
diff --git
a/src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java
b/src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java
index d2c2286..2eb3465 100644
---
a/src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java
+++
b/src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java
@@ -86,7 +86,8 @@ public class JobExecutionContextImpl implements
JobExecutionContext {
@Override
public void log(final String message, Object... args) {
- handler.persistJobProperties(handler.getJob().log(message, args));
+ final int logMaxCount = handler.getProgressLogMaxCount();
+ handler.persistJobProperties(handler.getJob().log(logMaxCount,
message, args));
}
@Override
diff --git a/src/test/java/org/apache/sling/event/impl/jobs/JobsImplTest.java
b/src/test/java/org/apache/sling/event/impl/jobs/JobsImplTest.java
index 4a540b0..496ca4f 100644
--- a/src/test/java/org/apache/sling/event/impl/jobs/JobsImplTest.java
+++ b/src/test/java/org/apache/sling/event/impl/jobs/JobsImplTest.java
@@ -19,6 +19,8 @@
package org.apache.sling.event.impl.jobs;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
import java.util.ArrayList;
import java.util.Calendar;
@@ -34,7 +36,7 @@ public class JobsImplTest {
@Test public void testSorting() {
final Calendar now = Calendar.getInstance();
- final Map<String, Object> properties = new HashMap<String, Object>();
+ final Map<String, Object> properties = new HashMap<>();
properties.put(Job.PROPERTY_JOB_CREATED, now);
final JobImpl job1 = new JobImpl("test", "hello_1", properties);
@@ -59,4 +61,100 @@ public class JobsImplTest {
assertEquals(job5, list.get(4));
}
+
+ @Test
+ public void testProgressLogCount() {
+ final Map<String, Object> properties = new HashMap<>();
+
+ final JobImpl job = new JobImpl("test", "hello_1", properties);
+
+ assertNull(job.getProperty(Job.PROPERTY_JOB_PROGRESS_LOG));
+ assertNull(job.getProgressLog());
+
+ for (int i = 0; i < 20; i++) {
+ job.log(10, "message_" + i);
+ }
+
+ final String[] progressLog = job.getProgressLog();
+ assertEquals(10, progressLog.length);
+ for (int i = 0; i < 9; i++) {
+ assertEquals("message_1" + i, progressLog[i]);
+ }
+ assertEquals("message_19" + JobImpl.TRUNCATED_LOG, progressLog[9]);
+ }
+
+ @Test
+ public void testProgressLogCountWithZeroCount() {
+ final Map<String, Object> properties = new HashMap<>();
+
+ final JobImpl job = new JobImpl("test", "hello_1", properties);
+
+ for (int i = 0; i < 2; i++) {
+ job.log(0, "message_" + i);
+ }
+
+ assertNull(job.getProgressLog());
+ }
+
+ @Test
+ public void testProgressLogCountWithNegativeCount() {
+ final Map<String, Object> properties = new HashMap<>();
+
+ final JobImpl job = new JobImpl("test", "hello_1", properties);
+
+ for (int i = 0; i < 2; i++) {
+ job.log(-2, "message_" + i);
+ }
+
+ assertNull(job.getProgressLog());
+ }
+
+ @Test
+ public void testProgressLogCountWithInfiniteCount() {
+ final Map<String, Object> properties = new HashMap<>();
+
+ final JobImpl job = new JobImpl("test", "hello_1", properties);
+
+ for (int i = 0; i < 20; i++) {
+ job.log(Integer.MAX_VALUE, "message_" + i);
+ }
+
+ final String[] progressLog = job.getProgressLog();
+ assertEquals(20, progressLog.length);
+ for (int i = 0; i < 20; i++) {
+ assertEquals("message_" + i, progressLog[i]);
+ }
+ }
+
+ @Test
+ public void testProgressLogCountMigrationFromOldJob() {
+ final Map<String, Object> properties = new HashMap<>();
+ properties.put(Job.PROPERTY_JOB_PROGRESS_LOG, new String[0]);
+
+ final JobImpl job = new JobImpl("test", "hello_1", properties);
+
+ assertTrue(job.getProperty(Job.PROPERTY_JOB_PROGRESS_LOG) instanceof
String[]);
+
+ for (int i = 0; i < 20; i++) {
+ job.log(Integer.MAX_VALUE, "message_" + i);
+ }
+
+ final String[] progressLog = job.getProgressLog();
+ assertEquals(20, progressLog.length);
+ for (int i = 0; i < 20; i++) {
+ assertEquals("message_" + i, progressLog[i]);
+ }
+
+ // now create a new Job with Max Count
+ final JobImpl newJob = new JobImpl("test", "hello_1", properties);
+ for (int i = 0; i < 20; i++) {
+ newJob.log(10, "newMessage_" + i);
+ }
+ final String[] newProgressLog = newJob.getProgressLog();
+ assertEquals(10, newProgressLog.length);
+ for (int i = 0; i < 9; i++) {
+ assertEquals("newMessage_1" + i, newProgressLog[i]);
+ }
+ assertEquals("newMessage_19" + JobImpl.TRUNCATED_LOG,
newProgressLog[9]);
+ }
}
diff --git
a/src/test/java/org/apache/sling/event/impl/jobs/config/JobManagerConfigurationTestFactory.java
b/src/test/java/org/apache/sling/event/impl/jobs/config/JobManagerConfigurationTestFactory.java
index 3ece2a3..24ffca7 100644
---
a/src/test/java/org/apache/sling/event/impl/jobs/config/JobManagerConfigurationTestFactory.java
+++
b/src/test/java/org/apache/sling/event/impl/jobs/config/JobManagerConfigurationTestFactory.java
@@ -52,7 +52,10 @@ public class JobManagerConfigurationTestFactory {
public int cleanup_period() {
return 0;
}
-
+
+ public int progresslog_maxCount() {
+ return Integer.MAX_VALUE;
+ }
});
return real;
}
