Make sure that spooling manager is loaded when enabled

core/trino-main/src/main/java/io/trino/server/protocol/spooling/PreferredQueryDataEncoderSelector.java

@@ -26,23 +26,16 @@ public class PreferredQueryDataEncoderSelector
 {
     private final Logger log = Logger.get(PreferredQueryDataEncoderSelector.class);
     private final QueryDataEncoders encoders;
-    private final SpoolingManagerRegistry spoolingManagerRegistry;
 
     @Inject
-    public PreferredQueryDataEncoderSelector(QueryDataEncoders encoders, SpoolingManagerRegistry spoolingManagerRegistry)
+    public PreferredQueryDataEncoderSelector(QueryDataEncoders encoders)
     {
         this.encoders = requireNonNull(encoders, "encoders is null");
-        this.spoolingManagerRegistry = requireNonNull(spoolingManagerRegistry, "spoolingManagerRegistry is null");
     }
 
     @Override
     public Optional<QueryDataEncoder.Factory> select(List<String> encodings)
     {
-        if (spoolingManagerRegistry.getSpoolingManager().isEmpty()) {
-            log.debug("Client requested one of the spooled encodings '%s' but spooling is disabled", encodings);
-            return Optional.empty();
-        }
-
         for (String encoding : encodings) {
             if (encoders.exists(encoding)) {
                 return Optional.of(encoders.get(encoding));

core/trino-main/src/main/java/io/trino/server/protocol/spooling/SpoolingManagerRegistry.java

@@ -13,7 +13,9 @@
  */
 package io.trino.server.protocol.spooling;
 
+import com.google.inject.ConfigurationException;
 import com.google.inject.Inject;
+import com.google.inject.spi.Message;
 import io.airlift.log.Logger;
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.trace.Tracer;
@@ -27,6 +29,7 @@
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.ConcurrentHashMap;
@@ -78,7 +81,7 @@ public void loadSpoolingManager()
         }
 
         if (!CONFIG_FILE.exists()) {
-            return;
+            throw new ConfigurationException(List.of(new Message("Spooling protocol is enabled, but manager configuration file does not exist: " + CONFIG_FILE)));
         }
 
         Map<String, String> properties = loadProperties();

core/trino-main/src/test/java/io/trino/server/protocol/spooling/TestPreferredQueryDataEncoderSelector.java

@@ -0,0 +1,73 @@
+/*
+ * Licensed 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 io.trino.server.protocol.spooling;
+
+import io.trino.server.protocol.spooling.QueryDataEncoder.EncoderSelector;
+import io.trino.server.protocol.spooling.encoding.JsonQueryDataEncoder;
+import org.junit.jupiter.api.Test;
+
+import java.util.List;
+import java.util.Set;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class TestPreferredQueryDataEncoderSelector
+{
+    @Test
+    public void testNoEncoderWhenNoneIsMatching()
+    {
+        EncoderSelector selector = new PreferredQueryDataEncoderSelector(new QueryDataEncoders(new SpoolingEnabledConfig().setEnabled(true), Set.of(new JsonQueryDataEncoder.Factory())));
+
+        assertThat(selector.select(List.of())).isEmpty();
+        assertThat(selector.select(List.of("json+zstd"))).isEmpty();
+    }
+
+    @Test
+    public void testNoEncoder()
+    {
+        EncoderSelector selector = new PreferredQueryDataEncoderSelector(new QueryDataEncoders(new SpoolingEnabledConfig().setEnabled(true), Set.of()));
+
+        assertThat(selector.select(List.of())).isEmpty();
+        assertThat(selector.select(List.of("json"))).isEmpty();
+    }
+
+    @Test
+    public void testSingleMatchingEncoderIsPicked()
+    {
+        JsonQueryDataEncoder.Factory factory = new JsonQueryDataEncoder.Factory();
+        EncoderSelector selector = new PreferredQueryDataEncoderSelector(new QueryDataEncoders(new SpoolingEnabledConfig().setEnabled(true), Set.of(factory)));
+
+        assertThat(selector.select(List.of("json+zstd", "json"))).hasValue(factory);
+    }
+
+    @Test
+    public void testSingleMatchingEncoderFromMultipleIsPicked()
+    {
+        JsonQueryDataEncoder.Factory factory = new JsonQueryDataEncoder.Factory();
+        EncoderSelector selector = new PreferredQueryDataEncoderSelector(new QueryDataEncoders(new SpoolingEnabledConfig().setEnabled(true), Set.of(factory, new JsonQueryDataEncoder.ZstdFactory(new QueryDataEncodingConfig()))));
+
+        assertThat(selector.select(List.of("protobuf", "json", "json+zstd"))).hasValue(factory);
+    }
+
+    @Test
+    public void testSingleMatchingEncoderFromMultipleIsPickedInOrder()
+    {
+        JsonQueryDataEncoder.Factory factory = new JsonQueryDataEncoder.Factory();
+        JsonQueryDataEncoder.ZstdFactory zstdFactory = new JsonQueryDataEncoder.ZstdFactory(new QueryDataEncodingConfig());
+
+        EncoderSelector selector = new PreferredQueryDataEncoderSelector(new QueryDataEncoders(new SpoolingEnabledConfig().setEnabled(true), Set.of(factory, zstdFactory)));
+
+        assertThat(selector.select(List.of("protobuf", "json+zstd", "json"))).hasValue(zstdFactory);
+    }
+}