[GitHub] [ignite-3] sergey-chugunov-1985 commented on a diff in pull request #1280: IGNITE-17935 Finish implementation of streaming RAFT snapshot sender

[GitBox <gi...@apache.org>]

sergey-chugunov-1985 commented on code in PR #1280:
URL: https://github.com/apache/ignite-3/pull/1280#discussion_r1012744025


##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/outgoing/OutgoingSnapshot.java:
##########
@@ -133,23 +140,61 @@ public PartitionKey partitionKey() {
     }
 
     /**
-     * Freezes the scope of this snapshot.
+     * Freezes the scope of this snapshot. This includes taking snapshot metadata and opening TX data cursor.
      *
      * <p>Must be called under snapshot lock.
      */
     void freezeScope() {
         assert mvOperationsLock.isLocked() : "MV operations lock must be acquired!";
 
+        frozenMeta = takeSnapshotMeta();
+
         txDataCursor = partition.txStatePartitionStorage().scan();
     }
 
+    private SnapshotMeta takeSnapshotMeta() {
+        long lastAppliedIndex = Math.max(
+                partition.mvPartitionStorage().lastAppliedIndex(),
+                partition.txStatePartitionStorage().lastAppliedIndex()
+        );
+
+        return SnapshotMetaUtils.snapshotMetaAt(lastAppliedIndex, logManager);
+    }
+
+    /**
+     * Returns metadata corresponding to this snapshot.
+     *
+     * @return This snapshot metadata.
+     */
+    public SnapshotMeta meta() {
+        SnapshotMeta meta = frozenMeta;
+
+        assert meta != null : "No snapshot meta yet, probably the snapshot scope was not yet frozen";
+
+        return meta;
+    }
+
     /**
-     * Reads a snapshot meta and returns a future with the response.
+     * Reads the snapshot meta and returns a future with the response.
      *
-     * @param metaRequest Meta request.
+     * @param request Meta request.
      */
-    SnapshotMetaResponse handleSnapshotMetaRequest(SnapshotMetaRequest metaRequest) {
-        //TODO https://issues.apache.org/jira/browse/IGNITE-17935
+    @Nullable
+    SnapshotMetaResponse handleSnapshotMetaRequest(SnapshotMetaRequest request) {
+        if (closed) {
+            return logAlreadyClosedAndReturnNull();
+        }
+
+        SnapshotMeta meta = frozenMeta;
+
+        assert meta != null : "No snapshot meta yet, probably the snapshot scope was not yet frozen";
+
+        return MESSAGES_FACTORY.snapshotMetaResponse().meta(meta).build();
+    }
+
+    @Nullable
+    private <T> T logAlreadyClosedAndReturnNull() {

Review Comment:
   @tkalkirill , in this case I side with Roman, assertion here is excessive.
   Two simple arguments from me:
   
   1. This method is merely a utility method to save a line of code in several places, it doesn't do something that may put stability of the system at risk.
   2. The method is private and context of its use is completely enclosed inside a rather short class. Assertions are a special tool to be used in rare situations where several properties or states of different components are supposed to have specific values. If we decide to put assertions in all methods like this one we'll end up with code where ```assert``` is the most used keyword in every class.
   
   What do you think?



-- 
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: notifications-unsubscribe@ignite.apache.org

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