phet commented on code in PR #3792:
URL: https://github.com/apache/gobblin/pull/3792#discussion_r1346381821
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiter.java:
##########
@@ -87,13 +89,14 @@ protected interface CheckedFunction<T, R> {
private final String constantsTableName;
private final int epsilon;
private final int linger;
+ private final int retention;
Review Comment:
this name is not descriptive (if it's a time unit, it should be long and
have a suffix for its units)
##########
gobblin-api/src/main/java/org/apache/gobblin/configuration/ConfigurationKeys.java:
##########
@@ -101,6 +101,8 @@ public class ConfigurationKeys {
public static final String DEFAULT_MULTI_ACTIVE_SCHEDULER_CONSTANTS_DB_TABLE
= "gobblin_multi_active_scheduler_constants_store";
public static final String SCHEDULER_LEASE_DETERMINATION_STORE_DB_TABLE_KEY
= MYSQL_LEASE_ARBITER_PREFIX + ".schedulerLeaseArbiter.store.db.table";
public static final String
DEFAULT_SCHEDULER_LEASE_DETERMINATION_STORE_DB_TABLE =
"gobblin_scheduler_lease_determination_store";
+ public static final String
SCHEDULER_LEASE_DETERMINATION_TABLE_RETENTION_PERIOD_MILLIS_KEY =
MYSQL_LEASE_ARBITER_PREFIX + ".retentionPeriodMillis";
+ public static final int
DEFAULT_SCHEDULER_LEASE_DETERMINATION_TABLE_RETENTION_PERIOD_MILLIS = 300000000;
Review Comment:
small nit: if you're not using `TimeUnit`, which is self-documenting:
```
TimeUnit.HOURS.toMillis(80);
TimeUnit.DAYS.toMillis(3);
```
please at least use time-esque values:
```
80 * 60 * 60 * 1000; // (80 hours in ms)
3 * 24 * 60 * 60 * 1000; // (3 days in ms)
```
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiter.java:
##########
@@ -221,6 +231,35 @@ private void initializeConstantsTable() throws IOException
{
}, true);
}
+ /**
+ * Periodically deletes all rows in the table with event_timestamp older
than the retention period defined by config.
+ */
+ private void runRetentionOnArbitrationTable() {
+ ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1);
+ Runnable retentionTask = () -> {
+ try {
+ Thread.sleep(10000);
+ withPreparedStatement(thisTableRetentionStatement,
+ retentionStatement -> {
+ int i = 0;
+ retentionStatement.setInt(++i, retention);
Review Comment:
`++i` seems obfuscated compared to hard-coding 1
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiter.java:
##########
@@ -221,6 +231,35 @@ private void initializeConstantsTable() throws IOException
{
}, true);
}
+ /**
+ * Periodically deletes all rows in the table with event_timestamp older
than the retention period defined by config.
+ */
+ private void runRetentionOnArbitrationTable() {
Review Comment:
seems worth abstracting into a utility along the lines of "run this
arbitrary SQL in a STPE using interval T"
you decide right now whether to do or merely "TODO" ;)
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MysqlMultiActiveLeaseArbiter.java:
##########
@@ -221,6 +231,35 @@ private void initializeConstantsTable() throws IOException
{
}, true);
}
+ /**
+ * Periodically deletes all rows in the table with event_timestamp older
than the retention period defined by config.
+ */
+ private void runRetentionOnArbitrationTable() {
+ ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1);
+ Runnable retentionTask = () -> {
+ try {
+ Thread.sleep(10000);
+ withPreparedStatement(thisTableRetentionStatement,
+ retentionStatement -> {
+ int i = 0;
+ retentionStatement.setInt(++i, retention);
+ int numRowsDeleted = retentionStatement.executeUpdate();
+ if (numRowsDeleted != 0) {
+ log.info("Multi-active lease arbiter retention thread deleted
{} rows from the lease arbiter table",
+ numRowsDeleted);
+ }
+ return numRowsDeleted;
+ }, true);
+ } catch (InterruptedException | IOException e) {
+ log.warn("Failing to run retention on lease arbiter table. Unbounded
growth can lead to database slowness and "
Review Comment:
`log.error`?
--
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]
