You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "ibessonov (via GitHub)" <gi...@apache.org> on 2024/02/19 07:19:41 UTC
[PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
ibessonov opened a new pull request, #3226:
URL: https://github.com/apache/ignite-3/pull/3226
https://issues.apache.org/jira/browse/IGNITE-21548
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494118340
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
+ return new Assignments(assignments).toBytes();
+ }
+
+ /**
+ * Deserializes assignments from the array of bytes.
+ */
+ public static Assignments fromBytes(byte[] bytes) {
+ return ByteUtils.fromBytes(bytes);
+ }
+
+ @Override
+ public String toString() {
+ return S.toString(this);
Review Comment:
Will it print anything useful?
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
Review Comment:
I think static methods will be enough.
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494125148
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
Review Comment:
I don't think so
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494336014
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
Review Comment:
In most cases there's no instance. In tests, in particular.
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494473039
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of nodes. */
+ @IgniteToStringInclude
+ private final Set<Assignment> nodes;
+
+ /**
+ * Constructor.
+ *
+ * @param nodes Set of nodes.
+ */
+ public Assignments(Set<Assignment> nodes) {
+ this.nodes = nodes;
+ }
+
+ /**
+ * Returns a set of nodes, represented by this assignments instance.
+ */
+ public Set<Assignment> nodes() {
+ return nodes;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
+ return new Assignments(assignments).toBytes();
+ }
+
+ /**
+ * Deserializes assignments from the array of bytes.
+ */
+ public static Assignments fromBytes(byte[] bytes) {
Review Comment:
I believe that we should return empty assignments in both cases. There's similar code below `entry.value() == null? NO_ASSIGNMENS : Assignments.fromBytes(entry.value())`. I believe that we should hide it insider fromBytes.
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494374605
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of nodes. */
+ @IgniteToStringInclude
+ private final Set<Assignment> nodes;
+
+ /**
+ * Constructor.
+ *
+ * @param nodes Set of nodes.
+ */
+ public Assignments(Set<Assignment> nodes) {
+ this.nodes = nodes;
+ }
+
+ /**
+ * Returns a set of nodes, represented by this assignments instance.
+ */
+ public Set<Assignment> nodes() {
+ return nodes;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
+ return new Assignments(assignments).toBytes();
+ }
+
+ /**
+ * Deserializes assignments from the array of bytes.
+ */
+ public static Assignments fromBytes(byte[] bytes) {
Review Comment:
Exceptions in both cases. I can handle nulls I guess
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494238084
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
Review Comment:
I see what you mean. Any ideas for a better name? `nodes` maybe?
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494468235
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
Review Comment:
Up to you.
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494176820
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
Review Comment:
It's up to you, but as far as I see you use static methods more often.
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494249163
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceUtil.java:
##########
@@ -186,22 +193,22 @@ public static CompletableFuture<Void> updatePendingAssignmentsKeys(
LOG.info(
"Update metastore pending partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPendingKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes));
+ partAssignments);
break;
case PLANNED_KEY_UPDATED:
LOG.info(
"Update metastore planned partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPlannedKey, partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ partAssignments
);
break;
case PLANNED_KEY_REMOVED_EQUALS_PENDING:
LOG.info(
"Remove planned key because current pending key has the same value [key={}, partition={}, table={}/{}, val={}]",
partAssignmentsPlannedKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ Assignments.fromBytes(partAssignmentsBytes)
Review Comment:
You didn't understand me. I meant, that you replaced similar code with simple `partAssignments` reference instead of deserialization in all similar places but this one. Why?
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov merged PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494243262
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
Review Comment:
If you already have an instance, you will be calling the other non-static method
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494124691
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
+ return new Assignments(assignments).toBytes();
+ }
+
+ /**
+ * Deserializes assignments from the array of bytes.
+ */
+ public static Assignments fromBytes(byte[] bytes) {
+ return ByteUtils.fromBytes(bytes);
+ }
+
+ @Override
+ public String toString() {
+ return S.toString(this);
Review Comment:
Yes, you might have noticed a "@IgniteToStringInclude" on the field "peers".
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494373654
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
Review Comment:
I have no real objections, but NO_ASSIGNMENTS is easier to use in static imports.
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494475133
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/AssignmentsTracker.java:
##########
@@ -97,7 +97,7 @@ public void startTrack() {
TablePartitionId grpId = TablePartitionId.fromString(strKey);
- Set<Assignment> assignments = ByteUtils.fromBytes(entry.value());
+ Set<Assignment> assignments = Assignments.fromBytes(entry.value()).nodes();
groupAssignments.put(grpId, assignments);
Review Comment:
Make sense.
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494376686
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/AssignmentsTracker.java:
##########
@@ -97,7 +97,7 @@ public void startTrack() {
TablePartitionId grpId = TablePartitionId.fromString(strKey);
- Set<Assignment> assignments = ByteUtils.fromBytes(entry.value());
+ Set<Assignment> assignments = Assignments.fromBytes(entry.value()).nodes();
groupAssignments.put(grpId, assignments);
Review Comment:
I'm planning to add meta-information to the Assignments class that won't be required for tracking. So this class is only for storage and communication
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2153,24 +2154,27 @@ protected CompletableFuture<Void> handleChangeStableAssignmentEvent(
TablePartitionId tablePartitionId = new TablePartitionId(tableId, partitionId);
- Set<Assignment> stableAssignments = ByteUtils.fromBytes(stableAssignmentsWatchEvent.value());
+ return supplyAsync(() -> {
+ Set<Assignment> stableAssignments = stableAssignmentsWatchEvent.value() == null
Review Comment:
Yes, I'll do it
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494235888
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
Review Comment:
It's for convenience. If you already have an instance, why would you create another one before serialization?
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494375061
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngineTest.java:
##########
@@ -384,8 +384,10 @@ void replicasTriggersAssignmentsChangingOnNonDefaultZones() throws Exception {
when(distributionZoneManager.dataNodes(anyLong(), anyInt(), anyInt())).thenReturn(completedFuture(Set.of("node0")));
+ byte[] assignmentsBytes = Assignments.toBytes(Set.of(Assignment.forPeer("node0")));
Review Comment:
Yes, why not
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494266132
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceUtil.java:
##########
@@ -186,22 +193,22 @@ public static CompletableFuture<Void> updatePendingAssignmentsKeys(
LOG.info(
"Update metastore pending partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPendingKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes));
+ partAssignments);
break;
case PLANNED_KEY_UPDATED:
LOG.info(
"Update metastore planned partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPlannedKey, partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ partAssignments
);
break;
case PLANNED_KEY_REMOVED_EQUALS_PENDING:
LOG.info(
"Remove planned key because current pending key has the same value [key={}, partition={}, table={}/{}, val={}]",
partAssignmentsPlannedKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ Assignments.fromBytes(partAssignmentsBytes)
Review Comment:
Oh, I guess I forgot to do it
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494238084
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
Review Comment:
I see what you mean. Any ideas for a better name?
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494237077
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceUtil.java:
##########
@@ -186,22 +193,22 @@ public static CompletableFuture<Void> updatePendingAssignmentsKeys(
LOG.info(
"Update metastore pending partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPendingKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes));
+ partAssignments);
break;
case PLANNED_KEY_UPDATED:
LOG.info(
"Update metastore planned partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPlannedKey, partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ partAssignments
);
break;
case PLANNED_KEY_REMOVED_EQUALS_PENDING:
LOG.info(
"Remove planned key because current pending key has the same value [key={}, partition={}, table={}/{}, val={}]",
partAssignmentsPlannedKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ Assignments.fromBytes(partAssignmentsBytes)
Review Comment:
We already have a class called `Assignment`. I don't want to introduce new naming, that's confusing
--
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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494316249
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of nodes. */
+ @IgniteToStringInclude
+ private final Set<Assignment> nodes;
+
+ /**
+ * Constructor.
+ *
+ * @param nodes Set of nodes.
+ */
+ public Assignments(Set<Assignment> nodes) {
Review Comment:
I'd rather add some sugar with either factory method `of()` or at least extra constructor `Assignments(Assignment ... nodes)`.
To be honest, private constructor and factory method seems better from my point of view. It will help with immutability. Please check the comment below.
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
Review Comment:
I'd rather name it EMPTY. Seems more common. Up to you.
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of nodes. */
+ @IgniteToStringInclude
+ private final Set<Assignment> nodes;
+
+ /**
+ * Constructor.
+ *
+ * @param nodes Set of nodes.
+ */
+ public Assignments(Set<Assignment> nodes) {
+ this.nodes = nodes;
+ }
+
+ /**
+ * Returns a set of nodes, represented by this assignments instance.
+ */
+ public Set<Assignment> nodes() {
Review Comment:
I'd rather make it immutable. Otherwise, we may have something like `assignments.nodes.add()`, which is actually not desired.
##########
modules/placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/AssignmentsTracker.java:
##########
@@ -97,7 +97,7 @@ public void startTrack() {
TablePartitionId grpId = TablePartitionId.fromString(strKey);
- Set<Assignment> assignments = ByteUtils.fromBytes(entry.value());
+ Set<Assignment> assignments = Assignments.fromBytes(entry.value()).nodes();
groupAssignments.put(grpId, assignments);
Review Comment:
Why do we still have Set<Assignment> instead of Assignments in groupAssignments?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2153,24 +2154,27 @@ protected CompletableFuture<Void> handleChangeStableAssignmentEvent(
TablePartitionId tablePartitionId = new TablePartitionId(tableId, partitionId);
- Set<Assignment> stableAssignments = ByteUtils.fromBytes(stableAssignmentsWatchEvent.value());
+ return supplyAsync(() -> {
+ Set<Assignment> stableAssignments = stableAssignmentsWatchEvent.value() == null
Review Comment:
Seems that we may handle null value inside Assignments.fromBytes itself.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -2153,24 +2154,27 @@ protected CompletableFuture<Void> handleChangeStableAssignmentEvent(
TablePartitionId tablePartitionId = new TablePartitionId(tableId, partitionId);
- Set<Assignment> stableAssignments = ByteUtils.fromBytes(stableAssignmentsWatchEvent.value());
+ return supplyAsync(() -> {
+ Set<Assignment> stableAssignments = stableAssignmentsWatchEvent.value() == null
+ ? emptySet()
+ : Assignments.fromBytes(stableAssignmentsWatchEvent.value()).nodes();
- return metaStorageMgr.get(pendingPartAssignmentsKey(tablePartitionId), revision)
- .thenComposeAsync(pendingAssignmentsEntry -> {
- byte[] pendingAssignmentsFromMetaStorage = pendingAssignmentsEntry.value();
+ Entry pendingAssignmentsEntry = metaStorageMgr.getLocally(pendingPartAssignmentsKey(tablePartitionId), revision);
- Set<Assignment> pendingAssignments = pendingAssignmentsFromMetaStorage == null
- ? Set.of()
- : ByteUtils.fromBytes(pendingAssignmentsFromMetaStorage);
+ byte[] pendingAssignmentsFromMetaStorage = pendingAssignmentsEntry.value();
- return stopAndDestroyPartitionAndUpdateClients(
- tablePartitionId,
- stableAssignments,
- pendingAssignments,
- isRecovery,
- revision
- );
- }, ioExecutor);
+ Assignments pendingAssignments = pendingAssignmentsFromMetaStorage == null
+ ? Assignments.NO_ASSIGNMENTS
Review Comment:
Same as above.
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of nodes and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Serial version UID. */
+ private static final long serialVersionUID = -59553172012153869L;
+
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of nodes. */
+ @IgniteToStringInclude
+ private final Set<Assignment> nodes;
+
+ /**
+ * Constructor.
+ *
+ * @param nodes Set of nodes.
+ */
+ public Assignments(Set<Assignment> nodes) {
+ this.nodes = nodes;
+ }
+
+ /**
+ * Returns a set of nodes, represented by this assignments instance.
+ */
+ public Set<Assignment> nodes() {
+ return nodes;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
+ return new Assignments(assignments).toBytes();
+ }
+
+ /**
+ * Deserializes assignments from the array of bytes.
+ */
+ public static Assignments fromBytes(byte[] bytes) {
Review Comment:
What do we get in case of 1) empty 2) null bytes?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/rebalance/DistributionZoneRebalanceEngineTest.java:
##########
@@ -384,8 +384,10 @@ void replicasTriggersAssignmentsChangingOnNonDefaultZones() throws Exception {
when(distributionZoneManager.dataNodes(anyLong(), anyInt(), anyInt())).thenReturn(completedFuture(Set.of("node0")));
+ byte[] assignmentsBytes = Assignments.toBytes(Set.of(Assignment.forPeer("node0")));
Review Comment:
Should be `Assignments.of(Assignment.of("node0"));`, don't 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
Re: [PR] IGNITE-21548 Encapsulate sets of assignments before storing them to meta-storage [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #3226:
URL: https://github.com/apache/ignite-3/pull/3226#discussion_r1494225844
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
+
+ /**
+ * Constructor.
+ *
+ * @param peers Set of peers.
+ */
+ public Assignments(Set<Assignment> peers) {
+ this.peers = peers;
+ }
+
+ /**
+ * Returns a set of peers, represented by this assignments instance.
+ */
+ public Set<Assignment> peers() {
+ return peers;
+ }
+
+ /**
+ * Serializes the instance into an array of bytes.
+ */
+ public byte[] toBytes() {
+ return ByteUtils.toBytes(this);
+ }
+
+ /**
+ * Serializes assignments into an array of bytes.
+ *
+ * @see #toBytes()
+ */
+ public static byte[] toBytes(Set<Assignment> assignments) {
Review Comment:
It's a matter of taste, but I think this method is redundant. I believe we should only have one way of doing the same thing.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceUtil.java:
##########
@@ -186,22 +193,22 @@ public static CompletableFuture<Void> updatePendingAssignmentsKeys(
LOG.info(
"Update metastore pending partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPendingKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes));
+ partAssignments);
break;
case PLANNED_KEY_UPDATED:
LOG.info(
"Update metastore planned partitions key [key={}, partition={}, table={}/{}, newVal={}]",
partAssignmentsPlannedKey, partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ partAssignments
);
break;
case PLANNED_KEY_REMOVED_EQUALS_PENDING:
LOG.info(
"Remove planned key because current pending key has the same value [key={}, partition={}, table={}/{}, val={}]",
partAssignmentsPlannedKey.toString(), partNum, tableDescriptor.id(), tableDescriptor.name(),
- ByteUtils.fromBytes(partAssignmentsBytes)
+ Assignments.fromBytes(partAssignmentsBytes)
Review Comment:
Why not `partAssignments`?
##########
modules/affinity/src/main/java/org/apache/ignite/internal/affinity/Assignments.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.affinity;
+
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.Set;
+import org.apache.ignite.internal.tostring.IgniteToStringInclude;
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.internal.util.ByteUtils;
+
+/**
+ * Class that encapsulates a set of peers and its metadata.
+ */
+public class Assignments implements Serializable {
+ /** Empty assignments. */
+ public static final Assignments NO_ASSIGNMENTS = new Assignments(Collections.emptySet());
+
+ /** Set of peers. */
+ @IgniteToStringInclude
+ private final Set<Assignment> peers;
Review Comment:
Why is this field called `peers`? Sounds strange to me
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1183,15 +1186,15 @@ private CompletableFuture<?> createTableLocally(
dataNodes,
zoneDescriptor.partitions(),
zoneDescriptor.replicas()
- ));
+ ).stream().map(assignments -> new Assignments(assignments)).collect(Collectors.toList()));
Review Comment:
```suggestion
).stream().map(Assignments::new).collect(Collectors.toList()));
```
--
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