[GitHub] [ignite-3] sanpwc commented on a diff in pull request #1538: IGNITE-18525 Introduce new PlacementDriver Ignite component

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

sanpwc commented on code in PR #1538:
URL: https://github.com/apache/ignite-3/pull/1538#discussion_r1072555675


##########
modules/placement-driver/build.gradle:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.
+ */
+
+apply from: "$rootDir/buildscripts/java-core.gradle"
+apply from: "$rootDir/buildscripts/java-junit5.gradle"
+apply from: "$rootDir/buildscripts/java-integration-test.gradle"
+apply from: "$rootDir/buildscripts/java-test-fixtures.gradle"
+
+dependencies {
+    implementation project(':ignite-metastorage-api')
+
+    implementation libs.jetbrains.annotations

Review Comment:
   I'd rather add libs part after all project part.



##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdrive/PlacementDriverManager.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.ignite.internal.placementdrive;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import org.apache.ignite.internal.util.IgniteSpinBusyLock;
+
+/**
+ * Placement driver manager.

Review Comment:
   That's totally useless)) Let's add some meaningful details here. We may reuse ones from README.MD



##########
modules/placement-driver/README.md:
##########
@@ -0,0 +1,13 @@
+# Placement driver
+
+The module responses for location of leaseholder node in the replication group. The placement driver decides a node which can hold a lease 

Review Comment:
   I'd rephrase this a bit. Let's mention that, on the one hand, placement driver is responsible for primary replica selection and fail-over and on the other hand placement driver provides an API in order to retrieve the information about available primary replicas along with corresponding meta such as leaseholder interval. 



##########
modules/placement-driver/README.md:
##########
@@ -0,0 +1,13 @@
+# Placement driver
+
+The module responses for location of leaseholder node in the replication group. The placement driver decides a node which can hold a lease 
+and approves with the replication group.
+
+## Leaseholder management
+Placement driver manager depends on Metastorage and a logical topology. The manager listens change of group members (due to a particular 

Review Comment:
   Let's also rephrase this, placement driver logically depends on the assignments and logical topology, there's no big deal that assignments are stored inside ms.



##########
modules/placement-driver/README.md:
##########
@@ -0,0 +1,13 @@
+# Placement driver
+
+The module responses for location of leaseholder node in the replication group. The placement driver decides a node which can hold a lease 
+and approves with the replication group.
+
+## Leaseholder management
+Placement driver manager depends on Metastorage and a logical topology. The manager listens change of group members (due to a particular 
+data zone shrink or growing) and appear / disappear nodes in logical topology. Every time when a new node become available (appeared in the 
+topology and added to the group members) or unavailable (disappeared from the topology or removed to the group members) Placement driver 
+choosing a leaseholder taking into these modifications.

Review Comment:
   > Every time when a new node become available (appeared in the 
   topology and added to the group members) or unavailable (disappeared from the topology or removed to the group members) Placement driver 
   choosing a leaseholder taking into these modifications.
   
   What do you mean?



##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdrive/PlacementDriverManager.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.ignite.internal.placementdrive;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.apache.ignite.internal.manager.IgniteComponent;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import org.apache.ignite.internal.util.IgniteSpinBusyLock;
+
+/**
+ * Placement driver manager.
+ */
+public class PlacementDriverManager implements IgniteComponent {

Review Comment:
   So you do expect some PlacementDriver to be managed or it's the service itself?



##########
modules/placement-driver/build.gradle:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.
+ */
+
+apply from: "$rootDir/buildscripts/java-core.gradle"
+apply from: "$rootDir/buildscripts/java-junit5.gradle"

Review Comment:
   apply from: "$rootDir/buildscripts/publishing.gradle" ?



##########
modules/placement-driver/README.md:
##########
@@ -0,0 +1,13 @@
+# Placement driver
+
+The module responses for location of leaseholder node in the replication group. The placement driver decides a node which can hold a lease 
+and approves with the replication group.
+
+## Leaseholder management
+Placement driver manager depends on Metastorage and a logical topology. The manager listens change of group members (due to a particular 
+data zone shrink or growing) and appear / disappear nodes in logical topology. Every time when a new node become available (appeared in the 
+topology and added to the group members) or unavailable (disappeared from the topology or removed to the group members) Placement driver 
+choosing a leaseholder taking into these modifications.
+
+The leaseholder will be able to be elected only when group members are enough. By the reason the module waits the members at first and then
+the defines a leaseholder of the group.

Review Comment:
   > The leaseholder will be able to be elected only when group members are enough. By the reason the module waits the members at first and then the defines a leaseholder of the group.
   
   Let's add more details here. I'd even add simple flow diagram here skipping the specifics of initial communication logic. 
   



-- 
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