homatthew commented on code in PR #3677:
URL: https://github.com/apache/gobblin/pull/3677#discussion_r1164767309
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java:
##########
@@ -112,22 +117,11 @@ public static String getHelixInstanceName(
return namePrefix + "_" + instanceId;
}
- // We have switched from Helix JobQueue to WorkFlow based job execution.
- @Deprecated
- public static void submitJobToQueue(
- JobConfig.Builder jobConfigBuilder,
- String queueName,
- String jobName,
- TaskDriver helixTaskDriver,
- HelixManager helixManager,
- long jobQueueDeleteTimeoutSeconds) throws Exception {
- submitJobToWorkFlow(jobConfigBuilder, queueName, jobName, helixTaskDriver,
helixManager, jobQueueDeleteTimeoutSeconds);
- }
-
static void waitJobInitialization(
HelixManager helixManager,
String workFlowName,
- String jobName) throws Exception {
+ String jobName,
+ Duration timeout) throws Exception {
Review Comment:
There's nothing inherently wrong with representing as a long. The downside
is there's no safeguards for passing in the wrong value when going from one
form of time (e.g. `Second` to `Millisecond`).
The resulting boiler plate becomes readable and "slightly" less error prone.
I will admit It's all nits at the end of the day
Let's consider this example
```
long timeoutSecond = ConfigUtil.getLong(...)
// option 1: multiply by magic number where the conversion isn't enforced by
the compiler
foobar(timeoutSecond * 1000)
// option 2: use native Duration java library which wouldn't let you forget
to do the conversion. And the conversion is implicit
foobarUsingDuration(Duration.ofSecond(timeoutSecond))
```
--
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]
