Yicong-Huang commented on code in PR #5300:
URL: https://github.com/apache/texera/pull/5300#discussion_r3330649985
##########
amber/src/main/scala/org/apache/texera/web/service/ExecutionsMetadataPersistService.scala:
##########
@@ -46,25 +46,27 @@ object ExecutionsMetadataPersistService extends LazyLogging
{
* This method inserts a new entry of a workflow execution in the database
and returns the generated eId
*
* @param workflowId the given workflow
- * @param uid user id that initiated the execution
+ * @param uid user id that initiated the execution; must be non-null
(uid is NOT NULL)
* @return generated execution ID
*/
def insertNewExecution(
workflowId: WorkflowIdentity,
- uid: Option[Integer],
+ uid: Integer,
executionName: String,
environmentVersion: String,
computingUnitId: Integer
): ExecutionIdentity = {
+ // uid is NOT NULL; reject up front instead of a cryptic jOOQ violation.
+ require(uid != null, "uid must be provided to persist a new workflow
execution")
Review Comment:
hmmm, In scala we use `Option[X]` to denote a variable could be type
`Some(X)` or `None`. This is to avoid the implicit `null` values in java. Such
None check can happen at compilation time and it is tighter.
Your change removes the `None` but falls back to `null`, this essentially
removes the compilation verification on None. That's why we need this runtime
`require` to check if the uid is null. I don't think this is the right
direction.
if we were to enforce `Interger`, we should make sure our frontend gives uid
all the time and no null values can be generated.
--
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]
