You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2024/10/27 10:53:32 UTC
[PR] IGNITE-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
tkalkirill opened a new pull request, #4637:
URL: https://github.com/apache/ignite-3/pull/4637
https://issues.apache.org/jira/browse/IGNITE-23546
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818487550
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItIdempotentCommandCacheTest.java:
##########
@@ -475,46 +467,6 @@ public void testIdempotentCacheRestoreFromSnapshot(IdempotentCommand idempotentC
}
}
- @Test
- void testEvictIdempotentCommandsCacheAfterCompaction() {
Review Comment:
Yep, you are right!
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818484004
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItIdempotentCommandCacheTest.java:
##########
@@ -475,46 +467,6 @@ public void testIdempotentCacheRestoreFromSnapshot(IdempotentCommand idempotentC
}
}
- @Test
- void testEvictIdempotentCommandsCacheAfterCompaction() {
Review Comment:
Is that correct that the only possibility to catch CompactionException on removeAll(commandIdsToEvict) is a removal attempt from an obsolete node, while another node already removed and compacted the commands?
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818480048
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/CommandResultAndTimestamp.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.metastorage.server.raft;
+
+import java.io.Serializable;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.jetbrains.annotations.Nullable;
+
+/** Helper class for idempotent command cache. */
+class CommandResultAndTimestamp {
+ final @Nullable Serializable commandResult;
Review Comment:
I'll leave it as is.
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818486429
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItIdempotentCommandCacheTest.java:
##########
@@ -475,46 +467,6 @@ public void testIdempotentCacheRestoreFromSnapshot(IdempotentCommand idempotentC
}
}
- @Test
- void testEvictIdempotentCommandsCacheAfterCompaction() {
Review Comment:
Commands that change metastorage never throws CompactedException.
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818477369
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItIdempotentCommandCacheTest.java:
##########
@@ -475,46 +467,6 @@ public void testIdempotentCacheRestoreFromSnapshot(IdempotentCommand idempotentC
}
}
- @Test
- void testEvictIdempotentCommandsCacheAfterCompaction() {
Review Comment:
The test checks that if there is a race between compaction and eviction from command cache, then we would make a fallback on the full scan (KeyValueStorage#range without upper bound) when reading. But since we don't do this anymore, the test is not needed in my opinion.
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818478622
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/CommandResultAndTimestamp.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.metastorage.server.raft;
+
+import java.io.Serializable;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.jetbrains.annotations.Nullable;
+
+/** Helper class for idempotent command cache. */
+class CommandResultAndTimestamp {
+ final @Nullable Serializable commandResult;
Review Comment:
Well, the fact that the class is package private doesn't mean that we touch fields directly. Not a java way I'd say. 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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818475367
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/CommandResultAndTimestamp.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.metastorage.server.raft;
+
+import java.io.Serializable;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.jetbrains.annotations.Nullable;
+
+/** Helper class for idempotent command cache. */
+class CommandResultAndTimestamp {
+ final @Nullable Serializable commandResult;
Review Comment:
Yes, they are used, but the fields are not private because the class is package private, I don’t see the need for getters.
--
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-23546 Fix long handling of EvictIdempotentCommandsCacheCommand [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4637:
URL: https://github.com/apache/ignite-3/pull/4637#discussion_r1818471399
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/raft/CommandResultAndTimestamp.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.metastorage.server.raft;
+
+import java.io.Serializable;
+import org.apache.ignite.internal.hlc.HybridTimestamp;
+import org.jetbrains.annotations.Nullable;
+
+/** Helper class for idempotent command cache. */
+class CommandResultAndTimestamp {
+ final @Nullable Serializable commandResult;
Review Comment:
Is it intended to use fields directly instead of private + getters?
##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItIdempotentCommandCacheTest.java:
##########
@@ -475,46 +467,6 @@ public void testIdempotentCacheRestoreFromSnapshot(IdempotentCommand idempotentC
}
}
- @Test
- void testEvictIdempotentCommandsCacheAfterCompaction() {
Review Comment:
It's suspicious. Why it's removed?
--
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