dimas-b commented on code in PR #3915:
URL: https://github.com/apache/polaris/pull/3915#discussion_r2897527418
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/IdempotencyRecord.java:
##########
@@ -112,13 +112,12 @@ public String responseSummary() {
}
/**
- * Serialized representation of a small, whitelisted set of HTTP response
headers.
+ * Allowlisted snapshot of HTTP response headers to replay for duplicates.
*
- * <p>Stored as a JSON string so that the HTTP layer can replay key headers
(such as {@code
- * Content-Type}) when serving a duplicate idempotent request.
+ * <p>How this map is persisted is store-specific.
*/
@Override
- public String responseHeaders() {
+ public Map<String, String> responseHeaders() {
Review Comment:
HTTP headers can have multiple values. Restricting this map to one value per
key may limit the ability of the Polaris Server to represent certain responses.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/idempotency/RelationalJdbcIdempotencyStoreFactory.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.
+ */
+package org.apache.polaris.persistence.relational.jdbc.idempotency;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.ApplicationScoped;
+import jakarta.enterprise.inject.Any;
+import jakarta.enterprise.inject.Instance;
+import jakarta.enterprise.inject.literal.NamedLiteral;
+import jakarta.inject.Inject;
+import java.sql.SQLException;
+import javax.sql.DataSource;
+import org.apache.polaris.core.persistence.IdempotencyStore;
+import org.apache.polaris.core.persistence.IdempotencyStoreFactory;
+import
org.apache.polaris.persistence.relational.jdbc.RelationalJdbcConfiguration;
+
+@ApplicationScoped
+@Identifier("relational-jdbc")
Review Comment:
nit: it might be best to defer adding CDI-related classes to the PR that
actually uses them.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/IdempotencyStore.java:
##########
@@ -131,29 +132,53 @@ ReserveResult reserve(
HeartbeatResult updateHeartbeat(
String realmId, String idempotencyKey, String executorId, Instant now);
+ /**
+ * Cancels (deletes) an in-progress idempotency reservation owned by {@code
executorId}.
+ *
+ * <p>This is used for cases where an idempotency key was reserved but the
request should not be
+ * tracked for replay (for example, 401/403/408/429 responses per the
proposal). Implementations
+ * should only cancel the record if it is still in-progress (i.e., not
finalized) and owned by the
+ * given executor.
+ *
+ * @param realmId logical tenant or realm identifier
+ * @param idempotencyKey application-provided idempotency key
+ * @param executorId identifier of the executor that owns the reservation
+ * @return {@code true} if a record was cancelled, {@code false} otherwise
Review Comment:
It would be nice to specify behaviour in case exceptions are thrown.
Does the caller care about the success of this operation? In other words, is
returning `true` critical?
Note: some exceptions may occur after the JDBC commit completes.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/IdempotencyStoreFactory.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.
+ */
+package org.apache.polaris.core.persistence;
+
+/**
+ * Factory for creating an {@link IdempotencyStore} backend.
+ *
+ * <p>Implementations are selected by the runtime wiring layer (e.g. Quarkus
CDI) using an
+ * identifier, similar to other Polaris factories.
+ */
+public interface IdempotencyStoreFactory {
+ /** Create an {@link IdempotencyStore} instance. */
+ IdempotencyStore create();
Review Comment:
This method does not appear to be called 🤔
What the intended usage patter for this factory?
--
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]
