sundapeng commented on code in PR #10351: URL: https://github.com/apache/paimon/pull/10351#discussion_r4368041399
########## paimon-python/pypaimon/filesystem/io_cache_file_io.py: ########## @@ -0,0 +1,304 @@ +# 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. + +"""FileIO that sends each request to an io-cache target or the origin endpoint.""" + +import threading +from typing import Callable, Dict, List, Optional + +import pyarrow.fs as pafs +from pyarrow._fs import FileSystemHandler + +from pypaimon.common.file_io import FileIO +from pypaimon.filesystem.io_cache_routing import IoCacheRouting, Op + + +class IoCacheRoutingFileIO(FileIO): + """Sends each call to origin or to a target FileIO, all built for the table path. + Calls and attributes that are not routed go to origin.""" + + def __init__(self, routing: IoCacheRouting, origin: FileIO, + create_target: Callable[[str], FileIO]): + self._routing = routing + self._origin = origin + self._create_target = create_target + self._targets: Dict[str, FileIO] = {} + self._targets_lock = threading.Lock() + self._filesystem = pafs.PyFileSystem(_IoCacheRoutingHandler(self)) + + @property + def properties(self): + return self._origin.properties + + @property + def filesystem(self): + """Filesystem for readers; it takes the URIs returned by to_filesystem_path.""" + return self._filesystem + + def _route(self, op: Op, path: str) -> Optional[str]: + return self._routing.route(op, path) + + def _file_io(self, name: Optional[str]) -> FileIO: + """Origin, or the FileIO of a target, created on first use.""" + if name is None: + return self._origin + file_io = self._targets.get(name) + if file_io is not None: + return file_io + with self._targets_lock: Review Comment: Thanks, bf0c925930 makes the routing FileIO check the pid before it touches the target lock. A forked child now starts with a new lock and an empty target map instead of waiting on a lock that a parent thread holds. `test_forked_child_does_not_wait_on_the_parents_target_lock` holds a parent thread inside target creation, forks, and reads in the child with 10 s bounds: without the fix the child is killed after 10 s and the test fails, with it the read returns at once. After the merge with master, RESTTokenFileIO resets its own locks in its at-fork hooks, and the routed FileIO it caches is the one that checks the pid. ########## paimon-python/pypaimon/read/reader/lance_utils.py: ########## @@ -78,7 +87,7 @@ def to_lance_specified(file_io: FileIO, file_path: str) -> Tuple[str, Optional[D if properties.contains(OssOptions.OSS_ENDPOINT): storage_options['oss_endpoint'] = properties.get(OssOptions.OSS_ENDPOINT) - storage_options['virtual_hosted_style_request'] = 'true' + storage_options['virtual_hosted_style_request'] = 'false' if path_style else 'true' Review Comment: Thanks. 8e65f00259 already gave Lance and Vortex the origin's options rather than a target's, since they open files with their own clients; only Paimon's own FileIO calls use targets. Since #10776 on master, `lance_utils.py` and the Lance reader are gone, and the remaining `export_lance` reads the table through Paimon's FileIO and writes with storage options the caller passes, so it never sees target options. The same check on Vortex found that a URL origin endpoint (`io-cache.origin.endpoint=https://...`) gave it `https://bucket.https://host`. 2bd427a35c fixes that, and `test_io_cache_routing_hands_vortex_the_origin` checks through RESTTokenFileIO that, with a path-style IP target, Vortex gets the origin's virtual-hosted endpoint and no target FileIO is created. -- 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]
