Abacn commented on code in PR #26023: URL: https://github.com/apache/beam/pull/26023#discussion_r1179434083
########## sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/Dependency.java: ########## @@ -0,0 +1,34 @@ +/* + * 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.beam.sdk.expansion.service; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.auto.value.AutoValue; + +//// TODO: generalize to support other types of dependencies Review Comment: Add a GitHub Issue link to this TODO? Is it possible to not suppress nullness warning here and below new codes? (received this request from other reviewers before) ########## runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/ExpansionServiceClient.java: ########## @@ -22,4 +22,7 @@ /** A high-level client for a cross-language expansion service. */ public interface ExpansionServiceClient extends AutoCloseable { ExpansionApi.ExpansionResponse expand(ExpansionApi.ExpansionRequest request); + + ExpansionApi.DiscoverSchemaTransformResponse discover( Review Comment: Just confirm, is this interface deemed for internal usage only, so adding a new method would not break classes that implemented it? ########## transform-service/controller/src/main/java/org/apache/beam/transformservice/controller/ExpansionService.java: ########## @@ -0,0 +1,159 @@ +/* + * 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.beam.transformservice.controller; + +import java.util.ArrayList; +import java.util.List; +import org.apache.beam.model.expansion.v1.ExpansionApi; +import org.apache.beam.model.expansion.v1.ExpansionServiceGrpc; +import org.apache.beam.model.pipeline.v1.Endpoints; +import org.apache.beam.runners.core.construction.DefaultExpansionServiceClientFactory; +import org.apache.beam.runners.core.construction.ExpansionServiceClientFactory; +import org.apache.beam.vendor.grpc.v1p54p0.io.grpc.ManagedChannelBuilder; +import org.apache.beam.vendor.grpc.v1p54p0.io.grpc.stub.StreamObserver; +import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.base.Throwables; +import org.checkerframework.checker.nullness.qual.Nullable; + +public class ExpansionService extends ExpansionServiceGrpc.ExpansionServiceImplBase + implements AutoCloseable { + + private static final ExpansionServiceClientFactory EXPANSION_SERVICE_CLIENT_FACTORY = + DefaultExpansionServiceClientFactory.create( + endPoint -> ManagedChannelBuilder.forTarget(endPoint.getUrl()).usePlaintext().build()); + + private final ExpansionServiceClientFactory expansionServiceClientFactory; + + final List<Endpoints.ApiServiceDescriptor> endpoints; + + ExpansionService( + List<Endpoints.ApiServiceDescriptor> endpoints, + @Nullable ExpansionServiceClientFactory clientFactory) { + this.endpoints = endpoints; + this.expansionServiceClientFactory = + clientFactory != null ? clientFactory : EXPANSION_SERVICE_CLIENT_FACTORY; + } + + @Override + public void expand( + ExpansionApi.ExpansionRequest request, + StreamObserver<ExpansionApi.ExpansionResponse> responseObserver) { + try { + responseObserver.onNext(processExpand(request)); + responseObserver.onCompleted(); + } catch (RuntimeException exn) { + responseObserver.onNext( Review Comment: Is it fine to suppress RuntimeException here, if yes may consider add any log, same below ########## transform-service/controller/src/main/java/org/apache/beam/transformservice/controller/ExpansionService.java: ########## @@ -0,0 +1,159 @@ +/* + * 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.beam.transformservice.controller; + +import java.util.ArrayList; +import java.util.List; +import org.apache.beam.model.expansion.v1.ExpansionApi; +import org.apache.beam.model.expansion.v1.ExpansionServiceGrpc; +import org.apache.beam.model.pipeline.v1.Endpoints; +import org.apache.beam.runners.core.construction.DefaultExpansionServiceClientFactory; +import org.apache.beam.runners.core.construction.ExpansionServiceClientFactory; +import org.apache.beam.vendor.grpc.v1p54p0.io.grpc.ManagedChannelBuilder; +import org.apache.beam.vendor.grpc.v1p54p0.io.grpc.stub.StreamObserver; +import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.base.Throwables; +import org.checkerframework.checker.nullness.qual.Nullable; + +public class ExpansionService extends ExpansionServiceGrpc.ExpansionServiceImplBase + implements AutoCloseable { + + private static final ExpansionServiceClientFactory EXPANSION_SERVICE_CLIENT_FACTORY = Review Comment: Naming: `DEFAULT_EXPANSION_SERVICE_CLIENT_FACTORY` or `DEFAULT_FACTORY` ? ########## transform-service/controller/build.gradle: ########## @@ -0,0 +1,109 @@ +/* + * 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. + */ + +import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar + +plugins { + id 'org.apache.beam.module' + id 'com.github.johnrengelman.shadow' +} + +apply plugin: 'org.apache.beam.module' +applyJavaNature( + automaticModuleName: 'org.apache.beam.transformservice.controller', +) + +applyDockerNature() +applyGoNature() + +description = "Apache Beam :: Transform Service :: Controller" + +configurations { + dockerDependency +} + +dependencies { + dockerDependency project(path: ":transform-service:controller", configuration: "shadow") + implementation project(path: ":model:pipeline", configuration: "shadow") + implementation project(path: ":model:job-management", configuration: "shadow") + implementation project(path: ":sdks:java:core", configuration: "shadow") + implementation project(path: ":runners:core-construction-java") + implementation project(path: ":sdks:java:fn-execution") + implementation library.java.vendored_grpc_1_54_0 + implementation library.java.vendored_guava_26_0_jre + implementation library.java.jackson_annotations + implementation library.java.jackson_databind + implementation library.java.jackson_dataformat_yaml + testImplementation library.java.junit + testImplementation library.java.mockito_core + testImplementation project(path: ":runners:java-fn-execution") +} + +goBuild { + goTargets = '*.go' // only build the immediate directory. + outputLocation = './build/target/launcher/${GOOS}_${GOARCH}/boot' +} + +task copyDockerfileDependencies(type: Copy) { + from configurations.dockerDependency + rename 'beam-transform-service-controller-.*.jar', 'beam-transform-service-controller.jar' + setDuplicatesStrategy(DuplicatesStrategy.INCLUDE) + into "build/target" +} + +task copyConfigFile(type: Copy) { + from "transform_service_config.yml" + into "build/target" +} + +docker { + name containerImageName( + name: project.docker_image_default_repo_prefix + "transform_service_controller", + root: project.rootProject.hasProperty(["docker-repository-root"]) ? + project.rootProject["docker-repository-root"] : + project.docker_image_default_repo_root, + tag: project.rootProject.hasProperty(["docker-tag"]) ? + project.rootProject["docker-tag"] : project.sdk_version) + // tags used by dockerTag task + tags containerImageTags() + files "./build" +} + +dockerPrepare.dependsOn goBuild +dockerPrepare.dependsOn copyConfigFile +dockerPrepare.dependsOn copyDockerfileDependencies + +if (project.rootProject.hasProperty(["docker-pull-licenses"])) { + def copyGolangLicenses = tasks.register("copyGolangLicenses", Copy) { + from "${project(':release:go-licenses:py').buildDir}/output" + into "build/target/go-licenses" + dependsOn ':release:go-licenses:py:createLicenses' + } + dockerPrepare.dependsOn copyGolangLicenses +} else { + def skipPullLicenses = tasks.register("skipPullLicenses", Exec) { + executable "sh" + // Touch a dummy file to ensure the directory exists. + args "-c", "mkdir -p build/target/go-licenses && touch build/target/go-licenses/skip" + } + dockerPrepare.dependsOn skipPullLicenses +} + +task pushAll { + dependsOn ":sdks:java:expansion-service:container:dockerPush" Review Comment: push all pushes a different project (`:sdks:java:expansion-service:container`) is this correct? ########## transform-service/controller/build.gradle: ########## @@ -0,0 +1,109 @@ +/* + * 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. + */ + +import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar Review Comment: Is this unused import? ########## transform-service/controller/src/main/java/org/apache/beam/transformservice/controller/Controller.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.beam.transformservice.controller; + +import java.util.ArrayList; +import java.util.List; +import org.apache.beam.model.pipeline.v1.Endpoints; +import org.apache.beam.sdk.options.PipelineOptions; +import org.apache.beam.sdk.options.PipelineOptionsFactory; +import org.apache.beam.vendor.grpc.v1p54p0.io.grpc.Server; +import org.apache.beam.vendor.grpc.v1p54p0.io.grpc.ServerBuilder; + +public class Controller { + + List<Endpoints.ApiServiceDescriptor> endpoints; + + private final TransformServiceOptions options; + + public Controller(String[] args) { + // We use PipelineOptions just a library for parsing arguments here. + this(PipelineOptionsFactory.fromArgs(args).create()); + for (String expansionService : options.getTransformServiceConfig().getExpansionservices()) { Review Comment: Should endpoints be initialized in `Controller(PipelineOptions opts)`, otherwise `Controller(String[] args)` has an initialized endpoints while `Controller(PipelineOptions opts)` has an empty endpoints. Both constructor are public. also, any javadoc is nice for this public class and is executable. ########## sdks/python/expansion-service-container/Dockerfile: ########## @@ -0,0 +1,72 @@ +############################################################################### +# 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. +############################################################################### + +# We just need to support one Python version supported by Beam. +# Picking the current default Beam Python version which is Python 3.8. +FROM python:3.8-bullseye as expansion-service Review Comment: I see other dockerfile in Beam repo uses `FROM python:3.8-slim` base image. Does slim base image suffice here? ########## transform-service/controller/src/main/java/org/apache/beam/transformservice/controller/package-info.java: ########## @@ -0,0 +1,28 @@ +/* + * 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. + */ + +/** + * TODO: add Javadoc. + * + * <p>TODO: details Review Comment: Note: TODO here ########## transform-service/controller/go.mod: ########## @@ -0,0 +1,20 @@ +// 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. + +module gin + +go 1.17 Review Comment: Same, then could avoid the need of changing these version number here on SDK or go version bump ########## transform-service/build.gradle: ########## @@ -0,0 +1,23 @@ +/* + * 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. + */ + +plugins { + id 'org.apache.beam.module' +} + +description = "Apache Beam :: Transform Service" Review Comment: It is an empty build file for now. Are there going to be content? If not could this build file be dropped? ########## sdks/python/apache_beam/runners/portability/expansion_service_main.py: ########## @@ -59,7 +59,7 @@ def main(argv): artifact_service.ArtifactRetrievalService( artifact_service.BeamFilesystemHandler(None).file_reader), server) - server.add_insecure_port('localhost:{}'.format(known_args.port)) + server.add_insecure_port('[::]:{}'.format(known_args.port)) Review Comment: For curiosity, what is `[::]` and why "localhost" no longer working? -- 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]
