absurdfarce commented on code in PR #2037: URL: https://github.com/apache/cassandra-java-driver/pull/2037#discussion_r2337916236
########## core/src/main/java/com/datastax/oss/driver/api/core/tracker/RequestIdGenerator.java: ########## @@ -0,0 +1,74 @@ +/* + * 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 com.datastax.oss.driver.api.core.tracker; + +import com.datastax.oss.driver.api.core.cql.Statement; +import com.datastax.oss.driver.api.core.session.Request; +import com.datastax.oss.protocol.internal.util.collection.NullAllowingImmutableMap; +import edu.umd.cs.findbugs.annotations.NonNull; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.util.Map; + +/** + * Interface responsible for generating request IDs. + * + * <p>Note that all request IDs have a parent/child relationship. A "parent ID" can loosely be + * thought of as encompassing a sequence of a request + any attendant retries, speculative + * executions etc. It's scope is identical to that of a {@link + * com.datastax.oss.driver.internal.core.cql.CqlRequestHandler}. A "request ID" represents a single + * request within this larger scope. Note that a request corresponding to a request ID may be + * retried; in that case the retry count will be appended to the corresponding identifier in the + * logs. + */ +public interface RequestIdGenerator { + /** + * Generates a unique identifier for the session request. This will be the identifier for the + * entire `session.execute()` call. This identifier will be added to logs, and propagated to + * request trackers. + * + * @return a unique identifier for the session request + */ + String getParentId(); + + /** + * Generates a unique identifier for the node request. This will be the identifier for the CQL + * request against a particular node. There can be one or more node requests for a single session + * request, due to retries or speculative executions. This identifier will be added to logs, and + * propagated to request trackers. + * + * @param statement the statement to be executed + * @param parentId the session request identifier + * @return a unique identifier for the node request + */ + String getRequestId(@NonNull Request statement, @NonNull String parentId); Review Comment: Perhaps the single largest outstanding issue from the other PR: the naming of these methods. @SiyaoIsHiding has argued for "getSessionRequestId" and "getNodeRequestId" to maintain consistency with the C# interface (discussed in more detail in [her comment](https://github.com/apache/cassandra-java-driver/pull/2037/files#r2101366329) elsewhere). I do _not_ like either of these names, in no small part because the "session request ID" (a) isn't bound to the Session instance (and in fact the Session can have a completely different name) and (b) it's not actually used as an identifier for an outgoing message. I'd argue that the names "getRequestId" and "getMessageId" are _perhaps_ a bit clearer since they relate to what's actually going on. The request ID is associated with a single Request (which may in fact require multiple messages based on retries, speculative execution etc.) while the message ID is very clearly associated with a single message to a single node. My intent with the names "getParentId" and "getRequestId" was to reinforce the parent-child relationship of these IDs but if we were really going to do that in a generic way this should probably have been "getParentId" and "getChildId"... and I agree that's kinda dumb. In the end I wound up going halfway with the parent-child thing with "getParentId" and the request/message split with "getRequestId" and the result is... just a mess. I'm not sure what @lukasz-antoniak thinks. :) I think I'm still arguing for getRequestId() and getMessageId(). I completely agree with @SiyaoIsHiding that we don't want to diverge from the public API for no reason but we're talking here about the API for request ID generators. I'm not expecting a huge number of new impls of this functionality, and even if we are the fact is the drivers represent discrete projects... and sometimes it may make more sense for the projects to diverge in naming if it's more sane for their impl. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
