phet commented on code in PR #3967:
URL: https://github.com/apache/gobblin/pull/3967#discussion_r1631789727
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/MysqlDagActionStore.java:
##########
@@ -163,17 +163,17 @@ public boolean deleteDagAction(DagAction dagAction)
throws IOException {
}
// TODO: later change this to getDagActions relating to a particular flow
execution if it makes sense
- private DagAction getDagActionWithRetry(String flowGroup, String flowName,
String flowExecutionId, String jobName, DagActionType dagActionType,
ExponentialBackoff exponentialBackoff)
+ private DagAction getDagActionWithRetry(String flowGroup, String flowName,
long flowExecutionId, String jobName, DagActionType dagActionType,
ExponentialBackoff exponentialBackoff)
throws IOException, SQLException {
return
dbStatementExecutor.withPreparedStatement(String.format(GET_STATEMENT,
tableName), getStatement -> {
int i = 0;
getStatement.setString(++i, flowGroup);
getStatement.setString(++i, flowName);
- getStatement.setString(++i, flowExecutionId);
+ getStatement.setString(++i, String.valueOf(flowExecutionId));
getStatement.setString(++i, dagActionType.toString());
try (ResultSet rs = getStatement.executeQuery()) {
if (rs.next()) {
- return new DagAction(rs.getString(1), rs.getString(2),
rs.getString(3), rs.getString(4), DagActionType.valueOf(rs.getString(5)));
+ return new DagAction(rs.getString(1), rs.getString(2),
rs.getLong(3), rs.getString(4), DagActionType.valueOf(rs.getString(5)));
Review Comment:
does the JDBC `ResultSet` allow this? I thought it might be necessary to do:
```
Long.valueOf(rs.getString(3))
```
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagActionReminderSchedulerTest.java:
##########
@@ -29,16 +29,18 @@
import org.testng.Assert;
import org.testng.annotations.Test;
+import com.google.common.base.Joiner;
+
import org.apache.gobblin.configuration.ConfigurationKeys;
public class DagActionReminderSchedulerTest {
String flowGroup = "fg";
String flowName = "fn";
- String flowExecutionId = "123";
+ long flowExecutionId = 123L;
String jobName = "jn";
- String expectedKey = String.join(".", flowGroup, flowName, flowExecutionId,
jobName,
- String.valueOf(DagActionStore.DagActionType.LAUNCH));
+ String expectedKey = Joiner.on("_").join(".", flowGroup, flowName,
flowExecutionId, jobName,
Review Comment:
I'm fuzzy on `Joiner`... would this join using `"_"` or `"."`?
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/DagProcessingEngineTest.java:
##########
@@ -129,9 +129,9 @@ public synchronized DagTask next() throws
NoSuchElementException {
throw new RuntimeException("Simulating an exception to stop the
thread!");
}
if (i % FAILING_DAGS_FREQUENCY == 0 ) {
- return new MockedDagTask(new DagActionStore.DagAction("fg-" + i, "fn-"
+ i, "1234" + i, "jn-" + i, DagActionStore.DagActionType.LAUNCH), true);
+ return new MockedDagTask(new DagActionStore.DagAction("fg-" + i, "fn-"
+ i, (1234L + i), "jn-" + i, DagActionStore.DagActionType.LAUNCH), true);
} else {
- return new MockedDagTask(new DagActionStore.DagAction("fg-" + i, "fn-"
+ i, "1234" + i, "jn-" + i, DagActionStore.DagActionType.LAUNCH), false);
+ return new MockedDagTask(new DagActionStore.DagAction("fg-" + i, "fn-"
+ i, (1234L + i), "jn-" + i, DagActionStore.DagActionType.LAUNCH), false);
Review Comment:
this was string catenation before, now addition... is that OK?
##########
gobblin-modules/gobblin-kafka-09/src/test/java/org/apache/gobblin/runtime/DagActionStoreChangeMonitorTest.java:
##########
@@ -136,7 +135,7 @@ public void tearDown() throws Exception {
@Test
public void testProcessMessageWithHeartbeatAndNullDagAction() throws
SpecNotFoundException {
Kafka09ConsumerClient.Kafka09ConsumerRecord consumerRecord =
- wrapDagActionStoreChangeEvent(OperationType.HEARTBEAT, "", "", "",
null);
+ wrapDagActionStoreChangeEvent(OperationType.HEARTBEAT, "", "", 123L,
null);
Review Comment:
why not refer to the `FLOW_EXECUTION_ID` constant?
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/FlowSpec.java:
##########
@@ -146,14 +150,13 @@ public static FlowSpec.Builder builder(URI catalogURI,
Properties flowProps) {
* @param key
* @param value
*/
- public synchronized void addProperty(String key, String value) {
+ public synchronized void addProperty(String key, Object value) {
Review Comment:
this could obscure errors. I'd be more comfortable w/ an overloaded
`addProperty(String, long)` method - would that compile and type check?
--
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]
