sven-weber-db commented on code in PR #55712: URL: https://github.com/apache/spark/pull/55712#discussion_r3199929324
########## udf/worker/core/src/main/scala/org/apache/spark/udf/worker/core/direct/DirectUDFWorkerManager.scala: ########## @@ -0,0 +1,45 @@ +/* + * 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.udf.worker.core.direct + +import org.apache.spark.udf.worker.UDFWorkerSpecification +import org.apache.spark.udf.worker.core.{UDFWorkerManager, WorkerDispatcher, WorkerLogger} + +class DirectUDFWorkerManager( Review Comment: >This seems to be a dead class not used anywhere, let's remove it This class is the class that would be consumed in `SparkEnv` as the current implementation of the `DispatcherManager` spawning direct workers. I agree it is not yet complete and its implementation will need to be changed once the gRPC protocol lands. However, it will have to exist. Therefore, I would propose to keep it with the current todo and to replace the implementation once your changes land. >Call it DispatcherManager, as it is a central place to hold the dispatchers Ok, happy to rename it. >Instead of creating the dispatcher here, register a created dispatcher from callsite I don't think this works as dispatchers depend on the `workerSpec`, which is a runtime value. Therefore, we cannot pass one instance of a dispatcher but we need to be able to generate different instances at runtime. We could introduce a `DispatcherFactory` as an additional abstraction that is passed to the `DispatcherManager` instead of using subclasses, if you prefer this. ########## udf/worker/core/src/main/scala/org/apache/spark/udf/worker/core/direct/DirectUDFWorkerManager.scala: ########## @@ -0,0 +1,45 @@ +/* + * 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.udf.worker.core.direct + +import org.apache.spark.udf.worker.UDFWorkerSpecification +import org.apache.spark.udf.worker.core.{UDFWorkerManager, WorkerDispatcher, WorkerLogger} + +class DirectUDFWorkerManager( Review Comment: >This seems to be a dead class not used anywhere, let's remove it This class is the class that would be consumed in `SparkEnv` as the current implementation of the `DispatcherManager` spawning direct workers. I agree it is not yet complete and its implementation will need to be changed once the gRPC protocol lands. However, it will have to exist. Therefore, I would propose to keep it with the current todo and to replace the implementation once your changes land. >Call it DispatcherManager, as it is a central place to hold the dispatchers Ok, happy to rename it. >Instead of creating the dispatcher here, register a created dispatcher from callsite I don't think this works as dispatchers depend on the `workerSpec`, which is a runtime value. Therefore, we cannot pass one instance of a dispatcher but we need to be able to generate different instances at runtime. We could introduce a `DispatcherFactory` as an additional abstraction that is passed to the `DispatcherManager` instead of using subclasses, if you prefer this. -- 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]
