adutra commented on code in PR #2602: URL: https://github.com/apache/polaris/pull/2602#discussion_r2384016935
########## runtime/service/src/main/java/org/apache/polaris/service/tracing/RequestIdGenerator.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.service.tracing; + +import com.google.common.annotations.VisibleForTesting; +import jakarta.enterprise.context.ApplicationScoped; +import java.util.UUID; +import java.util.concurrent.atomic.AtomicLong; + +@ApplicationScoped +public class RequestIdGenerator { Review Comment: It's not faulty in the sense that it would throw an exception, but it has some corner cases: 1. Threads A and B increment the counter past `COUNTER_SOFT_MAX` (line 34) 2. Threads A and B evaluate `currentCounter >= COUNTER_SOFT_MAX` to true (line 36) 3. Thread A enters the synchronized block, resets `baseDefaultUuid` and `COUNTER` 4. Thread B enters the synchronized block, only to find that `currentCounter >= COUNTER_SOFT_MAX` is now false => the object's monitor was acquired for nothing. Also: 1. Thread A increments counter to `COUNTER_SOFT_MAX` (line 34) 2. Thread B also increments counter, but enters the synchronized block first 3. Thread B resets `baseDefaultUuid` and `COUNTER` 4. Thread A reads `baseDefaultUuid` (line 35), finds the new value 5. Thread A creates a request ID with the new `baseDefaultUuid` but the old counter value => not dangerous, but weird. That being said, you are absolutely right that my revised version above is faulty as well. I apologize for that, I wrote it too quickly. What I had in mind is the typical pattern where each invocation of the update function returns a new, immutable state. The below implementation is correct, and even much simpler since the state can be a record: ```java @ApplicationScoped public class RequestIdGenerator { record State(String uuid, long counter) { State() { this(UUID.randomUUID().toString(), 1); } String requestId() { return String.format("%s:%019d", uuid, counter); } State increment() { return counter == Long.MAX_VALUE ? new State() : new State(uuid, counter + 1); } } final AtomicReference<State> state = new AtomicReference<>(new State()); public String generateRequestId() { return state.getAndUpdate(State::increment).requestId(); } } ``` -- 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]
