[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #33: Immutable state

["dcapwell (via GitHub)" <gi...@apache.org>]

dcapwell commented on code in PR #33:
URL: https://github.com/apache/cassandra-accord/pull/33#discussion_r1105118209


##########
accord-core/src/main/java/accord/api/ProgressLog.java:
##########
@@ -26,6 +26,7 @@
 import accord.local.Command;
 import accord.local.CommandStore;
 import accord.local.Node.Id;
+import accord.local.SafeCommandStore;

Review Comment:
   unused import



##########
accord-core/src/main/java/accord/api/VisibleForImplementation.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.api;
+
+/**
+ * Indicates an element exists or is more visible than otherwise needed to support implementations
+ */
+
+public @interface VisibleForImplementation

Review Comment:
   does this imply binary/source compatibility?  If so, what's the difference between this and normal `public`?
   
   If this is for documentation, then should add the following
   
   ```
   @Documented
   @Inherited
   @Retention(RetentionPolicy.SOURCE)
   ```
   
   This will scope the annotation to the source only, but also tell javadoc to annotate this as part of the public api
   
   ```
   $ javap -v -cp accord-core/build/classes/java/main/ accord.local.Command.SerializerSupport | grep VisibleForImplementation
   $
   ```
   
   ```
   $ grep VisibleForImplementation accord-core/build/docs/javadoc/accord/local/Command.SerializerSupport.html
   <pre><a href="../../accord/api/VisibleForImplementation.html" title="annotation in accord.api">@VisibleForImplementation</a>
   ```
   
   Without that change, the following is what we get
   
   
   ```
   $ javap -v -cp accord-core/build/classes/java/main/ accord.local.Command.SerializerSupport | grep VisibleForImplementation
   Picked up _JAVA_OPTIONS: -Djava.net.preferIPv4Stack=true
     #64 = Utf8               Laccord/api/VisibleForImplementation;
   $ grep VisibleForImplementation accord-core/build/docs/javadoc/accord/local/Command.SerializerSupport.html
   $
   ```



##########
accord-core/src/main/java/accord/impl/SimpleProgressLog.java:
##########
@@ -231,15 +233,15 @@ void run(Command command)
                                         // should have found enough information to apply the result, but in case we did not reset progress
                                         if (progress() == Investigating)
                                             setProgress(Expected);
-                                    });
+                                    }).begin(commandStore.agent());

Review Comment:
   I really wish we can improve our build to detect a `AsyncChain` returned without calling `begin`; I manually had to inspect every call site...



-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org