phet commented on code in PR #3700:
URL: https://github.com/apache/gobblin/pull/3700#discussion_r1211098334
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/DagActionStore.java:
##########
@@ -70,21 +73,23 @@ public DagAction(String flowGroup, String flowName, String
flowExecutionId, DagA
* @param flowGroup flow group for the dag action
* @param flowName flow name for the dag action
* @param flowExecutionId flow execution for the dag action
+ * @param dagActionValue the value of the dag action
* @throws IOException
* @return true if we successfully delete one record, return false if the
record does not exist
*/
- boolean deleteDagAction(String flowGroup, String flowName, String
flowExecutionId) throws IOException;
+ boolean deleteDagAction(String flowGroup, String flowName, String
flowExecutionId, DagActionValue dagActionValue) throws IOException;
/***
* Retrieve action value by the flow group, flow name and flow execution id
from the {@link DagActionStore}.
* @param flowGroup flow group for the dag action
* @param flowName flow name for the dag action
* @param flowExecutionId flow execution for the dag action
+ * @param dagActionValue the value of the dag action
* @throws IOException Exception in retrieving the {@link DagAction}.
* @throws SpecNotFoundException If {@link DagAction} being retrieved is not
present in store.
*/
- DagAction getDagAction(String flowGroup, String flowName, String
flowExecutionId) throws IOException, SpecNotFoundException,
-
SQLException;
+ DagAction getDagAction(String flowGroup, String flowName, String
flowExecutionId, DagActionValue dagActionValue)
Review Comment:
for the delete case especially, I wondered whether we'd already have a
`DagAction` on hand.
overall, and IMO unfortunately, we use quite little abstraction throughout
gobblin service. most emblematic is the regular use of `(String, String,
String)` or `(String, String, long)` to specify a flow execution ID. an
alternative impl, by contrast might combine a `FlowExecutionId` w/ a
`DagActionValue` to form a `DagAction`. that would be not only more succinct
and self-documenting, but also more typesafe. it's from this general
perspective that I prefer the signature:
```
deleteDagAction(DagAction)
```
to
```
deleteDagAction(String, String, String, DagActionValue)
```
--
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]
