pvary commented on code in PR #12298: URL: https://github.com/apache/iceberg/pull/12298#discussion_r1967301718
########## core/src/main/resources/META-INF/services/org.apache.iceberg.io.datafile.DataFileServiceRegistry$WriterService: ########## @@ -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. +# + +org.apache.iceberg.avro.Avro$WriterService Review Comment: Let's channel all of the SPI related comments/discussion here. The following suggestions were mentioned WRT the SPI solution: 1. Use Dyn classes/methods to register readers/writers - These readers/writers needed to be registered on engine level. We will have readers/writers for Spark/Flink. I think it would be good to open up these interfaces for other engines like Hive/Trino etc. This would mean that we need to keep the `register` method `public`. Also we have quite high number of readers already inside Iceberg (Generic/Arrow/Flink/Spark vectorized/Spark non-vectorized/Spark comet). So we might want to move the registration to the place where the reader created. 2. Adopting random file formats - I don't see a big issue here, as the registration requires the `FileFormat` which is an enum defined in Iceberg, so the users could not create random readers/writers. 3. ServiceLoader is an anti-pattern - The main reason behind this statement is usually that the dependencies are hidden. This is will be true for Dyn classes/method solution as well. 3. Issues with configuration files - TBH I haven't seen these, but this could be a valid issue That said, I see that the community has a strong opinion against the service loader solution, so I will move to the dyn classes methods solution. -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
