jiangzho commented on code in PR #8: URL: https://github.com/apache/spark-kubernetes-operator/pull/8#discussion_r1580245359
########## spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/RuntimeVersions.java: ########## @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.spark.k8s.operator.spec; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonInclude; +import io.fabric8.generator.annotation.Required; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Data; +import lombok.NoArgsConstructor; + +@Data +@NoArgsConstructor +@AllArgsConstructor +@Builder +@JsonInclude(JsonInclude.Include.NON_NULL) +@JsonIgnoreProperties(ignoreUnknown = true) +public class RuntimeVersions { + @Required protected SparkVersion sparkVersion; Review Comment: (combining reply to the review regarding `RuntimeVersion` below) I do see the valid cases for version support flexibility. The approach can be 1. Do not directly reject SparkApps with invalid / unsupported versions when when they are created. Allow them to be submitted and reconciled. 2. Add a "validation" step in reconcilers, which mark these apps as failed as applicable. - or use admission webhook for this purpose 3. Add a configurable mapping at operator reconciler configuration, so people can self-configure the supported versions, include pre-release versions for testing purpose when deploy the operator. - the default values for this mapping are covered by e2e. I'll make a commit make these a string type, but there are a few items to be called out with this approach: * User may self-configure an incompatible version, - which can be the intention for devs, but we need a way to clearly warn / indicate this to make sure people are mindful when they do so. - As of version 0.1, operator does not pull the official image (yet) for each app. v0.1 uses a single submission worker across all supported Spark versions. In this model, the required `RuntimeVersion` + `SparkVersion` act as a courtesy reminder to users that "these are the versions and builds that we verified from operator e2es and we therefore announced as supported". The plan was to add new versions after validation with e2e. - As in SPIP doc, in future versions we plan to work on enhanced multi-submission worker mode which actually use the corresponding version image to submit application (achieve actual version agnostic at the cost of holding a set of stand-by submission workers). By then, the version are really used to pull corresponding images. * Docs would need for version formatting with this string type, or we need a sanitizer (e.g. make sure people give `4.0.0` instead of `v4.0.0`, `v4_0_0`, `spark-4.0.0`, `apache-spark-4.0.0.` .etc) ########## spark-operator-api/src/main/java/org/apache/spark/k8s/operator/status/AttemptInfo.java: ########## @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.spark.k8s.operator.status; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonInclude; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Data; +import lombok.NoArgsConstructor; + +@Data +@NoArgsConstructor +@AllArgsConstructor +@Builder +@JsonInclude(JsonInclude.Include.NON_NULL) +@JsonIgnoreProperties(ignoreUnknown = true) +public class AttemptInfo { + @Builder.Default protected final Long id = 0L; + + public AttemptInfo createNextAttemptInfo() { Review Comment: No, this is not thread safe. It's to quickly create a new AttemptInfo based on existing one, e.g. when starting a new attempt. Members of `ApplicationStatus` are not exposed via setter for the sake of integrity - and people can be mindful that a new `ApplicationStatus` need to be created in order to update status history or start a new attempt. For example ``` // ApplicationStatus need to include currentState, stateHistory, currentAttemptInfo, previousAttemptInfo // when building status for a new attempt, currentAttempt becomes previous AttemptInfo nextAttemptInfo = currentAttemptInfo.createNextAttemptInfo(); ApplicationStatus nextAttemptStatus = new ApplicationStatus(updatedState, updatedStateHistory, nextAttemptInfo, currentAttemptInfo) ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
