You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "arjun4084346 (via GitHub)" <gi...@apache.org> on 2024/07/11 20:27:38 UTC
[PR] fix quartz not able to create non static inner class, create a separa… [gobblin]
arjun4084346 opened a new pull request, #3998:
URL: https://github.com/apache/gobblin/pull/3998
…te scheduler for reminder jobs
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-XXX
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674699667
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
Review Comment:
1. nit: in class javadoc, there's no need to note that it's a class. instead, just jump in to say sth like, "Stores info about a pending {@link DagAction} that needs..."
2. but overall, however, that description is inaccurate - nothing is stored! instead, AFAICT, this merely deserializes a `DagAction` to submit it for {@link DagManagement}.
3. hence naming-wise, even though *this is* a reminder, it would be clearest to name it after what it actually does, e.g. `ReaddDagActionJob`
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1675358208
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
+ */
+ public class ReminderJob implements Job {
Review Comment:
I'm not convinced the factory is actually that related to the `DagActionReminderScheduler`. I was more accepting the compromise that it could be one way for the latter to "share" its `DagManagement` with the many `ReminderJob`s. I was more inclined as well, given there's only one factory instance.
but, TBH I do find it preferable for the `ReminderJobFactory` also to be `static` and with a `private final DagManagement` (initialized by `@RequiredArgsConstructor`). that would be created as:
```
this.quartzScheduler.setJobFactory(new ReminderJobFactory(dagManagement));
```
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1675358208
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
+ */
+ public class ReminderJob implements Job {
Review Comment:
I'm not convinced the factory is actually that related to the `DagActionReminderScheduler`. I was more accepting the compromise that it would be a way for the latter to "share" its `DagManagement` with the many `ReminderJob`s. also there's only one factory.
but, TBH, it would be perfectly fine, and IMO preferable if the `ReminderJobFactory` were static and also held a `private final DagManagement` (w/ `@RequiredArgsConstructor`), so it could be created as:
```
this.quartzScheduler.setJobFactory(new ReminderJobFactory(dagManagement));
```
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "umustafi (via GitHub)" <gi...@apache.org>.
umustafi commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674683497
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -56,19 +59,28 @@
@Slf4j
@Singleton
public class DagActionReminderScheduler {
+ public static final String DAG_ACTION_REMINDER_SCHEDULER_NAME = "DagActionReminderScheduler";
public static final String RetryReminderKeyGroup = "RetryReminder";
public static final String DeadlineReminderKeyGroup = "DeadlineReminder";
- private final Scheduler quartzScheduler;
Review Comment:
why not private?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderSchedulerTest.java:
##########
@@ -109,4 +115,52 @@ public void testRemindersForMultipleFlowExecutions() throws SchedulerException {
this.dagActionReminderScheduler.unscheduleReminderJob(launchLeaseParams, true);
this.dagActionReminderScheduler.unscheduleReminderJob(launchLeaseParams2, true);
}
+
+ @Test
+ public void testScheduleReminder() throws SchedulerException, InterruptedException, IOException {
Review Comment:
add comment that ur testing creating of reminder with static method.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674800948
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
Review Comment:
addressed first two points. about the renaming, i think if it is renamed, it would hide the fact that it is indeed a reminder job
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674706380
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
+ */
+ public class ReminderJob implements Job {
Review Comment:
non-static inner classes are good for tightly-coupled abstractions. as you just found, they have some pitfalls. in this case, it feels more like a `static` inner class. just give it a `private final DagManagement` member and `@RequiredArgsCtor`
the `ReminderJobFactory` would still be non-`static`. it's the one that will take its `DagActionReminderScheduler.this.dagManagement` member and pass that along to the job's ctor.
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
Review Comment:
1. nit: in class javadoc, there's no need to note that it's a class. instead, just jump in to say sth like, "Stores info about a pending {@link DagAction} that needs..."
2. but overall, however, that description is inaccurate - nothing is stored! instead, AFAICT, this merely deserializes a `DagAction` to submit it for {@link DagManagement}.
3. hence naming-wise, even though this is a reminder, it might be clearer to name it after what it actually does, e.g. `ReaddDagActionJob`
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674729985
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -56,19 +59,28 @@
@Slf4j
@Singleton
public class DagActionReminderScheduler {
+ public static final String DAG_ACTION_REMINDER_SCHEDULER_NAME = "DagActionReminderScheduler";
public static final String RetryReminderKeyGroup = "RetryReminder";
public static final String DeadlineReminderKeyGroup = "DeadlineReminder";
- private final Scheduler quartzScheduler;
Review Comment:
VisibleForTesting
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674650332
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -94,46 +106,13 @@ public void unscheduleReminderJob(DagActionStore.LeaseParams leaseParams, boolea
}
}
- /**
- * Static class used to store information regarding a pending dagAction that needs to be revisited at a later time
- * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
- * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
- */
- public class ReminderJob implements Job {
Review Comment:
moved it in the end of the class so all the class methods can be together
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674706380
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
+ */
+ public class ReminderJob implements Job {
Review Comment:
non-static inner classes are good for tightly-coupled abstractions. as you just found, they have some pitfalls. in this case, it's not so closely coupled and hence seems better off as a `static` inner class. just give it a `private final DagManagement` member and `@RequiredArgsCtor`
the `ReminderJobFactory` however would still need to be non-`static`. it's the one that will take its (parent instance's) `DagActionReminderScheduler.this.dagManagement` member and pass that along to the job's ctor.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #3998:
URL: https://github.com/apache/gobblin/pull/3998#discussion_r1674788561
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderScheduler.java:
##########
@@ -192,4 +171,44 @@ public static Trigger createReminderJobTrigger(DagActionStore.LeaseParams leaseP
.startAt(new Date(getCurrentTimeMillis.get() + reminderDurationMillis))
.build();
}
+
+ public class ReminderJobFactory implements JobFactory {
+ @Override
+ public Job newJob(TriggerFiredBundle bundle, Scheduler scheduler) {
+ return new ReminderJob();
+ }
+ }
+
+ /**
+ * Class used to store information regarding a pending dagAction that needs to be revisited at a later time
+ * by {@link DagManagement} interface to re-attempt a lease on if it has not been completed by the previous owner.
+ * These jobs are scheduled and used by the {@link DagActionReminderScheduler}.
+ */
+ public class ReminderJob implements Job {
Review Comment:
so you are saying `ReminderJobFactory` is more tightly coupled with `DagActionReminderScheduler` than `ReminderJob` is to `DagActionReminderScheduler` ?
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2108] fix quartz not able to create non static inner class, create a separa… [gobblin]
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo merged PR #3998:
URL: https://github.com/apache/gobblin/pull/3998
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org