ifesdjeen commented on code in PR #222: URL: https://github.com/apache/cassandra-accord/pull/222#discussion_r2211427253
########## accord-core/src/main/java/accord/local/LoadKeysFor.java: ########## @@ -0,0 +1,28 @@ +/* + * 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 accord.local; + +/** + * For operations that need information associated with keys or ranges, + * this indicates whether the data will be queried, updated, or both. + */ +public enum LoadKeysFor +{ + WRITE, READ_WRITE, RECOVERY Review Comment: I do not fully understand: should it be queried (read?) -> updated (write) -> both (read/write)? I am assuming recovery is as strong as read/write, could you explain the distinction? ########## accord-core/src/main/java/accord/local/LoadKeysFor.java: ########## @@ -0,0 +1,28 @@ +/* + * 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 accord.local; + +/** + * For operations that need information associated with keys or ranges, + * this indicates whether the data will be queried, updated, or both. + */ +public enum LoadKeysFor +{ + WRITE, READ_WRITE, RECOVERY Review Comment: Also, it would be good to specify that write can not operate on ranges, which seems to be the whole point of the optimization. I think it is not entirely obvious from this context, or even without very close inspection. ########## accord-core/src/main/java/accord/local/PreLoadContext.java: ########## @@ -111,12 +114,14 @@ default boolean isSubsetOf(PreLoadContext superset) Unseekables<?> keys = keys(); if (!keys.isEmpty()) { - KeyHistory requiredHistory = keyHistory(); + LoadKeys requiredHistory = loadKeys(); Review Comment: I feel like we should rename `LoadKeys` now, since `LoadKeysFor` probably has meaning closer to this now, LoadKeys seems more like mode? ########## accord-core/src/main/java/accord/impl/AbstractSafeCommandStore.java: ########## @@ -81,8 +81,18 @@ public PreLoadContext canExecute(PreLoadContext with) if (with.keys().domain() == Routable.Domain.Range) return with.isSubsetOf(this.context) ? with : null; - if (!context().keyHistory().satisfiesIfPresent(with.keyHistory())) - return null; + { Review Comment: Should we unindent, or is this intentional? ########## accord-core/src/main/java/accord/impl/progresslog/DefaultProgressLog.java: ########## @@ -333,7 +333,7 @@ public void clearBefore(SafeCommandStore safeStore, TxnId clearWaitingBefore, Tx { // the command might be invalidated, which should be established on load, so simply load the command TxnId txnId = state.txnId; - safeStore.commandStore().execute(txnId, safeStore0 -> { + safeStore.commandStore().execute(PreLoadContext.contextFor(txnId, "Clear Progress"), safeStore0 -> { Review Comment: nit: will Java "intern" this for us? If not - should we create local runnable statics? ########## accord-core/src/main/java/accord/local/PreLoadContext.java: ########## @@ -91,15 +93,16 @@ default void forEachId(Consumer<TxnId> consumer) */ default Unseekables<?> keys() { return RoutingKeys.EMPTY; } - default KeyHistory keyHistory() { return NONE; } + default LoadKeys loadKeys() { return NONE; } + default LoadKeysFor loadKeysFor() { return WRITE; } Review Comment: Should we have a default here even? Could be surprising for an implementer, easy to overlook. Having to implement it at least would make a person think / consider. -- 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]
