jerryshao commented on code in PR #13474:
URL: https://github.com/apache/gravitino/pull/13474#discussion_r4080617512
##########
clients/client-python/gravitino/constants/error.py:
##########
@@ -68,6 +69,9 @@ class ErrorConstants(IntEnum):
# Error codes for drop an in use entity.
IN_USE_CODE = 1010
+ # Error codes for optimistic-lock conflicts.
+ OPTIMISTIC_LOCK_CONFLICT_CODE = 1012
Review Comment:
[Question] While closing the gap between the Java and Python code tables, is
it worth adding `UNAUTHORIZED_CODE = 1011` in the same change? Java has it
(common/src/main/java/org/apache/gravitino/dto/responses/ErrorConstants.java:58)
and `AuthenticationFilter` returns it for any failed authentication
(server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java:148),
but the Python enum still jumps from 1010 to 1012. That is not just a missing
mapping: `StatisticsErrorHandler.handle` converts the raw code with
`ErrorConstants(error_response.code())`
(clients/client-python/gravitino/exceptions/handlers/statistics_error_handler.py:21),
which raises `ValueError` for any code not in the enum.
Verified by: ran locally against this branch -
`STATISTICS_ERROR_HANDLER.handle(...)` on `{"code":1011,...}` produced
`ValueError: 1011 is not a valid ErrorConstants`, while the same response
through `REST_ERROR_HANDLER` produced a normal `RESTException`, and code 1012
now produces `OptimisticLockException` on both. Pre-existing, and out of the
stated scope of this PR - but a one-line addition here would fix it, and it is
the same class of gap the PR is closing.
##########
docs/how-to-use-gravitino-client.md:
##########
@@ -72,3 +72,11 @@ gravitino_client = GravitinoClient(
| `gravitino_client_request_timeout` | An optional client timeout in seconds.
| `10` | No |
**Note:** Invalid configuration properties will result in exceptions.
+
+## Retrying concurrent metadata changes
+
+If another writer changes metadata during an alter or drop, the server returns
HTTP 409
+with error code `1012`. The Java and Python clients raise
`OptimisticLockException`.
Review Comment:
[Nit] The rest of this page is example-driven, and the exception is not
importable from an obvious top-level package on the Python side:
`clients/client-python/gravitino/exceptions/__init__.py` is license-header
only, so callers need `from gravitino.exceptions.base import
OptimisticLockException`. Naming both import paths here
(`org.apache.gravitino.exceptions.OptimisticLockException` for Java) - ideally
with a small try/catch snippet - would save readers a grep.
Verified by: read clients/client-python/gravitino/exceptions/__init__.py (16
lines, no re-exports) and confirmed the Java class lives at
api/src/main/java/org/apache/gravitino/exceptions/OptimisticLockException.java:28.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/TestErrorHandlers.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.gravitino.client;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.function.Consumer;
+import org.apache.gravitino.dto.responses.ErrorResponse;
+import org.apache.gravitino.exceptions.OptimisticLockException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/** Tests REST error handling for optimistic-lock conflicts. */
+public class TestErrorHandlers {
+
+ @Test
+ public void testOptimisticLockConflictAcrossHandlers() {
+ ErrorResponse response =
+ ErrorResponse.optimisticLockConflict(
+ OptimisticLockException.class.getSimpleName(), "Concurrent
update", null);
+ List<Consumer<ErrorResponse>> handlers =
+ Arrays.asList(
+ ErrorHandlers.restErrorHandler(),
+ ErrorHandlers.tableErrorHandler(),
+ ErrorHandlers.viewErrorHandler(),
+ ErrorHandlers.partitionErrorHandler(),
+ ErrorHandlers.statisticsErrorHandler(),
+ ErrorHandlers.catalogErrorHandler());
Review Comment:
[Nit] This list pins six of the twenty-two handlers. The mapping actually
works for all of them because every subclass of `RestErrorHandler` ends its
switch with `default: super.accept(errorResponse)`, so what is really being
asserted is that delegation holds. A handler added later that throws directly
from its `default` branch (or adds its own `case` for 1012) would silently drop
out of the new behaviour without failing this test. Consider driving the loop
over all the public `ErrorHandlers.*ErrorHandler()` factories instead of a
hand-picked subset.
Verified by: scanned the `default:` branch of every handler class in
clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java
(lines 340-1496) - all 22 currently delegate to `super.accept`, and none
declares a `case` for `OPTIMISTIC_LOCK_CONFLICT_CODE`.
--
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]
