snazy commented on code in PR #724:
URL: https://github.com/apache/polaris/pull/724#discussion_r1925763780
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/BasePolarisCatalogTest.java:
##########
@@ -1458,6 +1461,55 @@ public void testDropTableWithPurgeDisabled() {
.hasMessageContaining(PolarisConfiguration.DROP_WITH_PURGE_ENABLED.key);
}
+ @Test
+ public void testRefreshIOForTableLike() {
+ // Enable ALLOW_SPECIFYING_FILE_IO_IMPL and disable
SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION
+ PolarisConfigurationStore configurationStore =
+ new DefaultConfigurationStore(
+ Map.of(
+ "ALLOW_SPECIFYING_FILE_IO_IMPL",
+ true,
+
PolarisConfiguration.SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION.key,
+ false));
+ try (MockedStatic<CatalogUtil> catalogUtil =
+ Mockito.mockStatic(CatalogUtil.class, Mockito.CALLS_REAL_METHODS)) {
Review Comment:
Oh, please do not mock anything static. I know this is not powermock, but
mocking static stuff requires some "interesting" instrumentation. You wrap the
call to CatalogUtil from DefaultFileIOFactory in a separate method and override
that method here. Something like:
```java
class DefaultFileIOFactory {
@VisibleForTesting
FileIO callCatalogUtil(params) {
return CatalogUtil.load...
}
}
```
and here
```java
new DefaultFileIOFactory(params) {
@Override
FileIO callCatalogUtil(params) {
// run your assertions
return super.callCatalogUtil(...) // or not :shrug:
}
```
##########
service/common/src/main/java/org/apache/polaris/service/task/TaskFileIOSupplier.java:
##########
@@ -83,6 +83,6 @@ public FileIO apply(TaskEntity task, RealmId realmId) {
String ioImpl =
properties.getOrDefault(
CatalogProperties.FILE_IO_IMPL,
"org.apache.iceberg.io.ResolvingFileIO");
- return fileIOFactory.loadFileIO(ioImpl, properties);
+ return fileIOFactory.loadFileIO(ioImpl, properties, null, null, null,
null);
Review Comment:
Same comment about `null` as above
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -2040,7 +2034,13 @@ private List<TableIdentifier>
listTableLike(PolarisEntitySubType subType, Namesp
*/
private FileIO loadFileIO(String ioImpl, Map<String, String> properties) {
Map<String, String> propertiesWithS3CustomizedClientFactory = new
HashMap<>(properties);
- return fileIOFactory.loadFileIO(ioImpl,
propertiesWithS3CustomizedClientFactory);
+ return fileIOFactory.loadFileIO(
+ ioImpl,
+ propertiesWithS3CustomizedClientFactory,
+ null /*tableIdentifier*/,
+ null /*tableLocations*/,
+ null /*storageActions*/,
+ null /*resolvedStorageEntity*/);
Review Comment:
`null` looks a bit dangerous to me. Maybe add a
`loadFileIO(ioImpl,properties)` and let the implementation do the "right" thing?
##########
gradle/libs.versions.toml:
##########
@@ -76,6 +76,7 @@ junit-bom = { module = "org.junit:junit-bom", version =
"5.11.4" }
logback-classic = { module = "ch.qos.logback:logback-classic", version =
"1.5.16" }
micrometer-bom = { module = "io.micrometer:micrometer-bom", version = "1.14.3"
}
mockito-core = { module = "org.mockito:mockito-core", version = "5.15.2" }
+mockito-inline = { module = "org.mockito:mockito-inline", version = "5.2.0" }
Review Comment:
Hm. This one looks quite unmaintained - last release was 2023. Do we really
need it?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/io/FileIOFactory.java:
##########
@@ -19,9 +19,19 @@
package org.apache.polaris.service.catalog.io;
import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.io.FileIO;
+import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper;
+import org.apache.polaris.core.storage.PolarisStorageActions;
/** Interface for providing a way to construct FileIO objects, such as for
reading/writing S3. */
public interface FileIOFactory {
- FileIO loadFileIO(String impl, Map<String, String> properties);
+ FileIO loadFileIO(
+ String ioImplClassName,
+ Map<String, String> properties,
+ TableIdentifier identifier,
+ Set<String> tableLocations,
+ Set<PolarisStorageActions> storageActions,
+ PolarisResolvedPathWrapper resolvedStorageEntity);
Review Comment:
Can you add `@Nonnull` to the parameters? (see also my other comment about
`null`)
--
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]
