You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/08/31 10:27:34 UTC
[GitHub] [ignite] RodionSmolnikovGG opened a new pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
RodionSmolnikovGG opened a new pull request #9362:
URL: https://github.com/apache/ignite/pull/9362
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] 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 pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _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.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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
[GitHub] [ignite] ascherbakoff commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r760040930
##########
File path: modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/TxCommands.java
##########
@@ -138,8 +139,12 @@ else if (args.getOperation() == VisorTxOperation.KILL)
", consistentId=" + key.consistentId() +
"]");
- for (VisorTxInfo info : entry.getValue().getInfos())
- logger.info(info.toUserString());
+ for (VisorTxInfo info : entry.getValue().getInfos()) {
+ if (Objects.equals(info.getXid(), info.getNearXid()))
+ logger.info(info.toUserString());
+ else
+ logger.info(info.toUserString() + " [Near node may be missed]");
+ }
Review comment:
This is true. I suggest to collect only near transactions if non verbose mode.
##########
File path: modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/TxCommands.java
##########
@@ -138,8 +139,12 @@ else if (args.getOperation() == VisorTxOperation.KILL)
", consistentId=" + key.consistentId() +
"]");
- for (VisorTxInfo info : entry.getValue().getInfos())
- logger.info(info.toUserString());
+ for (VisorTxInfo info : entry.getValue().getInfos()) {
+ if (Objects.equals(info.getXid(), info.getNearXid()))
+ logger.info(info.toUserString());
+ else
+ logger.info(info.toUserString() + " [Near node may be missed]");
Review comment:
This message is not correct. The meaning of this branch condition - we are printing a non-near (local or remote) transaction. It has no relation to a presence of a node in the cluster.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -173,21 +179,22 @@
it.remove();
}
}
+
+ if (infos.size() > limit)
+ infos.subList(limit, infos.size()).clear();
}
}
return mapRes;
}
- /**
- *
- */
+ /** */
private static class VisorTxJob extends VisorJob<VisorTxTaskArg, VisorTxTaskResult> {
/** */
private static final long serialVersionUID = 0L;
/** */
- private static final int DEFAULT_LIMIT = 50;
+ private static final int DEFAULT_MAP_LIMIT = 50;
Review comment:
The limit should be applied only on reducer side and removed from map side. Transactions should be streamed to reducer in batches using continous mapping to avoid memory issues.
--
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
[GitHub] [ignite] DirectXceriD commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
DirectXceriD commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r760395261
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -325,15 +335,15 @@ else if (locTx instanceof GridDhtTxRemote) {
TxVerboseInfo verboseInfo = arg.verboseMode() ? createVerboseInfo(ignite, locTx) : null;
+ if (arg.getOperation() == VisorTxOperation.KILL)
Review comment:
I believe it's ok to have it this way, if we don't break the logic. It looks like an improvement to make this output less misleading.
--
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
[GitHub] [ignite] RodionSmolnikovGG commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
RodionSmolnikovGG commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r706060304
##########
File path: modules/core/src/test/java/org/apache/ignite/testsuites/IgniteVisorTaskTestSuite.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.testsuites;
+
+import org.apache.ignite.internal.visor.tx.VisorTxTaskExecutionTest;
+import org.apache.ignite.internal.visor.tx.VisorTxTaskTest;
+import org.junit.runner.RunWith;
+import org.junit.runners.Suite;
+
+/**
+ * Test suite for Visor task execution.
+ */
+@RunWith(Suite.class)
+@Suite.SuiteClasses({
+ VisorTxTaskTest.class,
+ VisorTxTaskExecutionTest.class
+})
+public class IgniteVisorTaskTestSuite {
Review comment:
will move to other suite
--
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
[GitHub] [ignite] vmalin commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
vmalin commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r702832727
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/visor/tx/VisorTxTaskExecutionTest.java
##########
@@ -0,0 +1,178 @@
+/*
+ * 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.visor.tx;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.NodeStoppingException;
+import org.apache.ignite.internal.TestRecordingCommunicationSpi;
+import org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxFinishRequest;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.visor.VisorTaskArgument;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.apache.ignite.transactions.Transaction;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheWriteSynchronizationMode.FULL_SYNC;
+import static org.apache.ignite.internal.TestRecordingCommunicationSpi.spi;
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+
+/**
+ * VisorTxTask integration test
+ */
+public class VisorTxTaskExecutionTest extends GridCommonAbstractTest {
+ /** Cache name. */
+ protected static final String CACHE_NAME = "test";
+
+ /** Server node count. */
+ private static final int GRID_CNT = 3;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+ cfg.setCommunicationSpi(new TestRecordingCommunicationSpi());
+
+ if (!igniteInstanceName.startsWith("client")) {
+ CacheConfiguration ccfg = new CacheConfiguration(CACHE_NAME);
+
+ ccfg.setAtomicityMode(TRANSACTIONAL);
+ ccfg.setBackups(GRID_CNT - 1);
+ ccfg.setWriteSynchronizationMode(FULL_SYNC);
+ ccfg.setOnheapCacheEnabled(false);
+
+ cfg.setCacheConfiguration(ccfg);
+ }
+
+ return cfg;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ final IgniteEx crd = startGrid(0);
+
+ startGridsMultiThreaded(1, GRID_CNT - 1);
+
+ crd.cluster().active(true);
Review comment:
_active(true)_ is deprecated
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/visor/tx/VisorTxTaskExecutionTest.java
##########
@@ -0,0 +1,178 @@
+/*
+ * 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.visor.tx;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.NodeStoppingException;
+import org.apache.ignite.internal.TestRecordingCommunicationSpi;
+import org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxFinishRequest;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.visor.VisorTaskArgument;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.apache.ignite.transactions.Transaction;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheWriteSynchronizationMode.FULL_SYNC;
+import static org.apache.ignite.internal.TestRecordingCommunicationSpi.spi;
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+
+/**
+ * VisorTxTask integration test
+ */
+public class VisorTxTaskExecutionTest extends GridCommonAbstractTest {
+ /** Cache name. */
+ protected static final String CACHE_NAME = "test";
+
+ /** Server node count. */
+ private static final int GRID_CNT = 3;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+ cfg.setCommunicationSpi(new TestRecordingCommunicationSpi());
+
+ if (!igniteInstanceName.startsWith("client")) {
+ CacheConfiguration ccfg = new CacheConfiguration(CACHE_NAME);
+
+ ccfg.setAtomicityMode(TRANSACTIONAL);
+ ccfg.setBackups(GRID_CNT - 1);
+ ccfg.setWriteSynchronizationMode(FULL_SYNC);
+ ccfg.setOnheapCacheEnabled(false);
+
+ cfg.setCacheConfiguration(ccfg);
+ }
+
+ return cfg;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ final IgniteEx crd = startGrid(0);
+
+ startGridsMultiThreaded(1, GRID_CNT - 1);
+
+ crd.cluster().active(true);
+
+ awaitPartitionMapExchange();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ super.afterTest();
Review comment:
It's better to call _super.afterTest()_ at the end of the current _afterTest()_. The idea is to first call more specific ending actions and then more general. _beforeTest()_ works vice versa.
##########
File path: modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/TxCommands.java
##########
@@ -138,8 +139,12 @@ else if (args.getOperation() == VisorTxOperation.KILL)
", consistentId=" + key.consistentId() +
"]");
- for (VisorTxInfo info : entry.getValue().getInfos())
- logger.info(info.toUserString());
+ for (VisorTxInfo info : entry.getValue().getInfos()) {
+ if (Objects.equals(info.getXid(), info.getNearXid()))
+ logger.info(info.toUserString());
+ else
+ logger.info(info.toUserString() + " [Near node may be missed]");
Review comment:
Near node **might** be miss**ing**. Or better "Near node might have left topology." Left **the** topology is grammatically correct, but _left toology_ is used in many places including logs.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -74,6 +73,9 @@
/** */
private static final long serialVersionUID = 0L;
+ /** */
+ private static final int DEFAULT_REDUCE_LIMIT = 5;
Review comment:
The logic looks a bit flawed to me. You use different values for map and reduce limits if the limit isn't set explicitly. However, if it is, you use the same value. I think default values should be equal. Otherwise, we will do some computations that will be discarded at the end(either in _map_ or in _reduce_ methods)
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/visor/tx/VisorTxTaskExecutionTest.java
##########
@@ -0,0 +1,178 @@
+/*
+ * 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.visor.tx;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.NodeStoppingException;
+import org.apache.ignite.internal.TestRecordingCommunicationSpi;
+import org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxFinishRequest;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.visor.VisorTaskArgument;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.apache.ignite.transactions.Transaction;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheWriteSynchronizationMode.FULL_SYNC;
+import static org.apache.ignite.internal.TestRecordingCommunicationSpi.spi;
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+
+/**
+ * VisorTxTask integration test
+ */
+public class VisorTxTaskExecutionTest extends GridCommonAbstractTest {
+ /** Cache name. */
+ protected static final String CACHE_NAME = "test";
+
+ /** Server node count. */
+ private static final int GRID_CNT = 3;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+ cfg.setCommunicationSpi(new TestRecordingCommunicationSpi());
+
+ if (!igniteInstanceName.startsWith("client")) {
+ CacheConfiguration ccfg = new CacheConfiguration(CACHE_NAME);
+
+ ccfg.setAtomicityMode(TRANSACTIONAL);
+ ccfg.setBackups(GRID_CNT - 1);
+ ccfg.setWriteSynchronizationMode(FULL_SYNC);
+ ccfg.setOnheapCacheEnabled(false);
+
+ cfg.setCacheConfiguration(ccfg);
+ }
+
+ return cfg;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ final IgniteEx crd = startGrid(0);
+
+ startGridsMultiThreaded(1, GRID_CNT - 1);
+
+ crd.cluster().active(true);
+
+ awaitPartitionMapExchange();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ super.afterTest();
+
+ stopAllGrids();
+ }
+
+ /**
+ * @return Started client.
+ * @throws Exception If f nodeailed.
+ */
+ private Ignite startClient() throws Exception {
+ Ignite client = startClientGrid("client");
+
+ assertTrue(client.configuration().isClientMode());
+
+ assertNotNull(client.cache(CACHE_NAME));
+
+ return client;
+ }
+
+
+ /** Test limit parameter for VisorTxTask */
+ @Test
+ public void testVisorTxTaskLimitParam() throws Exception {
+ final int txCnt = 50;
+ final long latchTimeoutSeconds = 60;
+ final int testLimit = 10;
+
+ final Ignite client = startClient();
+
+ final List<Integer> keys = primaryKeys(grid(0).cache(CACHE_NAME), txCnt);
+
+ CountDownLatch txLatch = new CountDownLatch(txCnt);
+ CountDownLatch commitLatch = new CountDownLatch(1);
+
+ List<IgniteInternalFuture> futures = new ArrayList<>(txCnt);
+
+ for (int i = 0; i < txCnt; i++) {
+
+ final int f = i;
+
+ futures.add(runAsync(new Runnable() {
Review comment:
No need to explicitly use _new Runnable()_. () -> construction will also work
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -217,7 +224,10 @@ private VisorTxJob(VisorTxTaskArg arg, boolean debug) {
List<VisorTxInfo> infos = new ArrayList<>();
- int limit = arg.getLimit() == null ? DEFAULT_LIMIT : arg.getLimit();
+ int perNodelimit = DEFAULT_MAP_LIMIT;
+
Review comment:
may be shortened:
int perNodeLimit = arg.getLimit == null ? DEFAULT_MAP_LIMIT : max(arg.getLimit, DEFAULT_MAP_LIMIT);
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -325,15 +335,15 @@ else if (locTx instanceof GridDhtTxRemote) {
TxVerboseInfo verboseInfo = arg.verboseMode() ? createVerboseInfo(ignite, locTx) : null;
+ if (arg.getOperation() == VisorTxOperation.KILL)
Review comment:
Is there any reason to move this string up?
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -120,7 +122,11 @@
/** {@inheritDoc} */
@Nullable @Override protected Map<ClusterNode, VisorTxTaskResult> reduce0(List<ComputeJobResult> results) throws IgniteException {
- Map<ClusterNode, VisorTxTaskResult> mapRes = new TreeMap<>();
+ int limit = taskArg.getLimit() == null ? DEFAULT_REDUCE_LIMIT : taskArg.getLimit();
+
+ if (limit <= 0) return Collections.emptyMap();
Review comment:
_return_ statement should occupy a separate line
##########
File path: modules/core/src/test/java/org/apache/ignite/testsuites/IgniteVisorTaskTestSuite.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.testsuites;
+
+import org.apache.ignite.internal.visor.tx.VisorTxTaskExecutionTest;
+import org.apache.ignite.internal.visor.tx.VisorTxTaskTest;
+import org.junit.runner.RunWith;
+import org.junit.runners.Suite;
+
+/**
+ * Test suite for Visor task execution.
+ */
+@RunWith(Suite.class)
+@Suite.SuiteClasses({
+ VisorTxTaskTest.class,
+ VisorTxTaskExecutionTest.class
+})
+public class IgniteVisorTaskTestSuite {
Review comment:
I can't see the new tests in the _Run all_ used to get a visa. Seems that something more should be done to register a new suite
##########
File path: modules/core/src/test/java/org/apache/ignite/internal/visor/tx/VisorTxTaskExecutionTest.java
##########
@@ -0,0 +1,178 @@
+/*
+ * 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.visor.tx;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import org.apache.ignite.Ignite;
+import org.apache.ignite.cluster.ClusterNode;
+import org.apache.ignite.configuration.CacheConfiguration;
+import org.apache.ignite.configuration.IgniteConfiguration;
+import org.apache.ignite.internal.IgniteEx;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import org.apache.ignite.internal.NodeStoppingException;
+import org.apache.ignite.internal.TestRecordingCommunicationSpi;
+import org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxFinishRequest;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.U;
+import org.apache.ignite.internal.visor.VisorTaskArgument;
+import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest;
+import org.apache.ignite.transactions.Transaction;
+import org.junit.Test;
+
+import static org.apache.ignite.cache.CacheAtomicityMode.TRANSACTIONAL;
+import static org.apache.ignite.cache.CacheWriteSynchronizationMode.FULL_SYNC;
+import static org.apache.ignite.internal.TestRecordingCommunicationSpi.spi;
+import static org.apache.ignite.testframework.GridTestUtils.runAsync;
+
+/**
+ * VisorTxTask integration test
+ */
+public class VisorTxTaskExecutionTest extends GridCommonAbstractTest {
+ /** Cache name. */
+ protected static final String CACHE_NAME = "test";
+
+ /** Server node count. */
+ private static final int GRID_CNT = 3;
+
+ /** {@inheritDoc} */
+ @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
+ IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
+
+ cfg.setCommunicationSpi(new TestRecordingCommunicationSpi());
+
+ if (!igniteInstanceName.startsWith("client")) {
+ CacheConfiguration ccfg = new CacheConfiguration(CACHE_NAME);
+
+ ccfg.setAtomicityMode(TRANSACTIONAL);
+ ccfg.setBackups(GRID_CNT - 1);
+ ccfg.setWriteSynchronizationMode(FULL_SYNC);
+ ccfg.setOnheapCacheEnabled(false);
+
+ cfg.setCacheConfiguration(ccfg);
+ }
+
+ return cfg;
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTest() throws Exception {
+ super.beforeTest();
+
+ final IgniteEx crd = startGrid(0);
+
+ startGridsMultiThreaded(1, GRID_CNT - 1);
+
+ crd.cluster().active(true);
+
+ awaitPartitionMapExchange();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void afterTest() throws Exception {
+ super.afterTest();
+
+ stopAllGrids();
+ }
+
+ /**
+ * @return Started client.
+ * @throws Exception If f nodeailed.
+ */
+ private Ignite startClient() throws Exception {
+ Ignite client = startClientGrid("client");
+
+ assertTrue(client.configuration().isClientMode());
+
+ assertNotNull(client.cache(CACHE_NAME));
+
+ return client;
+ }
+
+
+ /** Test limit parameter for VisorTxTask */
+ @Test
+ public void testVisorTxTaskLimitParam() throws Exception {
+ final int txCnt = 50;
+ final long latchTimeoutSeconds = 60;
+ final int testLimit = 10;
+
+ final Ignite client = startClient();
+
+ final List<Integer> keys = primaryKeys(grid(0).cache(CACHE_NAME), txCnt);
+
+ CountDownLatch txLatch = new CountDownLatch(txCnt);
+ CountDownLatch commitLatch = new CountDownLatch(1);
+
+ List<IgniteInternalFuture> futures = new ArrayList<>(txCnt);
+
+ for (int i = 0; i < txCnt; i++) {
+
+ final int f = i;
+
+ futures.add(runAsync(new Runnable() {
+ @Override public void run() {
+ try (Transaction tx = client.transactions().txStart()) {
+ client.cache(CACHE_NAME).put(keys.get(f), 0);
+
+ txLatch.countDown();
+
+ U.await(commitLatch, latchTimeoutSeconds, TimeUnit.SECONDS);
+
+ tx.commit();
+ }
+ catch (Exception e) {
+ if (!(e.getCause() instanceof NodeStoppingException))
+ log.error("Error while transaction executing.", e);
+
+ //NodeStoppingException expected.
+ }
+ }
+ }));
+ }
+
+ spi(client).blockMessages((node, msg) -> msg instanceof GridNearTxFinishRequest);
+
+ U.awaitQuiet(txLatch);
+
+ commitLatch.countDown();
+
+ VisorTxTaskArg arg =
+ new VisorTxTaskArg(VisorTxOperation.INFO, testLimit, null, null, null, null, null, null, null, null, null);
+
+ Map<ClusterNode, VisorTxTaskResult> res = client.compute(client.cluster().forPredicate(F.alwaysTrue())).
+ execute(new VisorTxTask(), new VisorTaskArgument<>(client.cluster().localNode().id(), arg, false));
+
+ //All transactions are in PREPARED state.
+ for (Map.Entry<ClusterNode, VisorTxTaskResult> entry : res.entrySet()) {
+ if (entry.getValue().getInfos().isEmpty())
+ fail("Every node should have transaction info.");
+
+ assertEquals(testLimit, entry.getValue().getInfos().size());
+ }
+
+ arg = new VisorTxTaskArg(VisorTxOperation.KILL, null, null, null, null, null, null, null, null, null, null);
+
+ client.compute(client.cluster().forPredicate(F.alwaysTrue()))
Review comment:
The result of this compute operation should also be checked.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -120,7 +122,11 @@
/** {@inheritDoc} */
@Nullable @Override protected Map<ClusterNode, VisorTxTaskResult> reduce0(List<ComputeJobResult> results) throws IgniteException {
- Map<ClusterNode, VisorTxTaskResult> mapRes = new TreeMap<>();
+ int limit = taskArg.getLimit() == null ? DEFAULT_REDUCE_LIMIT : taskArg.getLimit();
+
+ if (limit <= 0) return Collections.emptyMap();
+
+ Map<ClusterNode, VisorTxTaskResult> mapRes = new HashMap<>();
Review comment:
You changed the data structure here. If there is no good reason for this I strongly advise to leave it as it was.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -217,7 +224,10 @@ private VisorTxJob(VisorTxTaskArg arg, boolean debug) {
List<VisorTxInfo> infos = new ArrayList<>();
- int limit = arg.getLimit() == null ? DEFAULT_LIMIT : arg.getLimit();
+ int perNodelimit = DEFAULT_MAP_LIMIT;
Review comment:
perNode**L**imit
##########
File path: modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/TxCommands.java
##########
@@ -138,8 +139,12 @@ else if (args.getOperation() == VisorTxOperation.KILL)
", consistentId=" + key.consistentId() +
"]");
- for (VisorTxInfo info : entry.getValue().getInfos())
- logger.info(info.toUserString());
+ for (VisorTxInfo info : entry.getValue().getInfos()) {
+ if (Objects.equals(info.getXid(), info.getNearXid()))
+ logger.info(info.toUserString());
+ else
+ logger.info(info.toUserString() + " [Near node may be missed]");
+ }
Review comment:
Also looks like that a remark about missing node is correct only if the mode is non-verbose. In verbose mode we don't remove transactions that meet this condition.
--
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
[GitHub] [ignite] RodionSmolnikovGG commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
RodionSmolnikovGG commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r703003516
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -217,7 +224,10 @@ private VisorTxJob(VisorTxTaskArg arg, boolean debug) {
List<VisorTxInfo> infos = new ArrayList<>();
- int limit = arg.getLimit() == null ? DEFAULT_LIMIT : arg.getLimit();
+ int perNodelimit = DEFAULT_MAP_LIMIT;
+
Review comment:
Don't think it is better about readability
--
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
[GitHub] [ignite] RodionSmolnikovGG commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
RodionSmolnikovGG commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r703002519
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -120,7 +122,11 @@
/** {@inheritDoc} */
@Nullable @Override protected Map<ClusterNode, VisorTxTaskResult> reduce0(List<ComputeJobResult> results) throws IgniteException {
- Map<ClusterNode, VisorTxTaskResult> mapRes = new TreeMap<>();
+ int limit = taskArg.getLimit() == null ? DEFAULT_REDUCE_LIMIT : taskArg.getLimit();
+
+ if (limit <= 0) return Collections.emptyMap();
+
+ Map<ClusterNode, VisorTxTaskResult> mapRes = new HashMap<>();
Review comment:
Real mistake, sorry, reverted
--
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
[GitHub] [ignite] RodionSmolnikovGG commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
RodionSmolnikovGG commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r703005123
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/visor/tx/VisorTxTask.java
##########
@@ -325,15 +335,15 @@ else if (locTx instanceof GridDhtTxRemote) {
TxVerboseInfo verboseInfo = arg.verboseMode() ? createVerboseInfo(ignite, locTx) : null;
+ if (arg.getOperation() == VisorTxOperation.KILL)
Review comment:
If kill row is plased after infos addiction - output of the command will contain text with PREPARED or COMMITING statses. But user, i think prefer looks for a "ROLLBACKED" status after kill command execution
--
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
[GitHub] [ignite] RodionSmolnikovGG commented on a change in pull request #9362: IGNITE-9386 Fix cofused results of control.sh --tx when limit is set to a small number
Posted by GitBox <gi...@apache.org>.
RodionSmolnikovGG commented on a change in pull request #9362:
URL: https://github.com/apache/ignite/pull/9362#discussion_r703001465
##########
File path: modules/control-utility/src/main/java/org/apache/ignite/internal/commandline/TxCommands.java
##########
@@ -138,8 +139,12 @@ else if (args.getOperation() == VisorTxOperation.KILL)
", consistentId=" + key.consistentId() +
"]");
- for (VisorTxInfo info : entry.getValue().getInfos())
- logger.info(info.toUserString());
+ for (VisorTxInfo info : entry.getValue().getInfos()) {
+ if (Objects.equals(info.getXid(), info.getNearXid()))
+ logger.info(info.toUserString());
+ else
+ logger.info(info.toUserString() + " [Near node may be missed]");
Review comment:
Set "Near node might has left topology."
--
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