Claudenw commented on PR #13357: URL: https://github.com/apache/iceberg/pull/13357#issuecomment-3018350902
@kumarpritam863 There are several issues converging in this change. The issue with JdbcCatalog is an example. If we look at the code for the connector, it follows the standard process for generic connectors: save the config, make a copy of the config for each task, and let the system to the rest. The underlying system then creates the specified number of tasks by creating an instance of the task class and calling initialize with the context and then start with the properties. We expect the Catalog to be initialized at some point. The earliest point that this can be done is where I have placed it in this PR, in the `connector.start()` method. The only other reasonable place to initialize the Catalog is in the `task.start()` method. The problem arises when the Catalog has both a globally shared and a local component like the JdbcCatalog does. In the JdbcCatalog case there are two tables that must exist. The catalog initialization (after my patch) does the following: 1. Checks to see if the tables are available. 2. If not attempt to create the tables 3. if create failed check again to see if the tables are there. 4. if not fail. 5. read/write data to the catalog. At each of those points the thread may be switched out and processing stopped. What has happened in several cases that I have seen is that multiple threads execute step 1 but are switched out before executing step 2. The first thread back completes step 2 and works as expected. All other threads that executed the first step, fail in the second and succeed in the 3rd. So within one machine running multiple threads we execute several requests over the wire in an attempt to configure. It works but it increases the time necessary to start and tends to add spurious log messages about failed table creation. In the JDBC case, if we call the initialization early and from one thread before multiple threads are spun up it will create the tables once and then subsequent calls from the tasks will see the tables in step 1 and continue with step 5. In addition if the configuration fails then the connector fails before the tasks are created saving significant time and resources. Also, note that if there are multiple servers with multiple threads the number of calls increases significantly and the longer failure prone path can be taken even if the thread is not switched out just by having another system create the tables at the right (wrong?) moment. I am aware that this discussion relates only to JdbcCatalog, but it is an exemplar for any catalog that attempts to establish a global configuration. I also realize could be solved in the JDBC case by requiring that the tables be created before starting the system, or by using "create table" commands that use the "if exists" key word. The later was discussed in detail and has to be rejected (see issue https://github.com/apache/iceberg/issues/13343). I also note that there are, as far as I can tell, no requirement for the Catalog implementation other than have a no argument constructor, and an `initialize()` method that takes a name and a map of properties. So with no way to interrogate Catalogs to understand what their requirements are the only way to make it more efficient at startup is to make a call to initialize the Catalog is when the connector saves the configuration. (as per the change here). The cost of doing this is minimal. This change does two things: 1. Allows Catalogs that have global data to create that data once. 2. Allows a Catalog failure to be detected early. It has multiple advantages: 1. Reduces the time and resource consumption during failed some Catalog initializations. 2. Reduces the chance of resource conflict for Catalogs that have global data. -- 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]
