-
Notifications
You must be signed in to change notification settings - Fork 1.9k
IGNITE-27033: checkpoint control.sh command #12547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| import org.apache.ignite.internal.management.api.ComputeCommand; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| public class CheckpointForceCommand implements ComputeCommand<CheckpointForceCommandArg, String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic must be in CheckpointCommand itself.
So the desired syntax is:
> ./control.sh --checkpoint
> ./control.sh --checkpoint --reason "After data load checkpoint"
> ./control.sh --checkoint --wait-for-finish --timeout 60000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override protected String run(@Nullable CheckpointForceCommandArg arg) throws IgniteException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must check if persistence enabled on node with the:
if (!CU.isPersistenceEnabled(ignite.configuration())) {
throw new IgniteException("Can't checkpoint on in-memory node"); // Or return some result here.
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
@DEADripER please, fix code style violations. |
| import org.apache.ignite.internal.management.api.ComputeCommand; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| /** Checkpoint command class*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: /** Checkpoint command class*/ -> /** Checkpoint command. */
| CheckpointProgress checkpointfut = dbMgr.forceCheckpoint(reason); | ||
|
|
||
| if (waitForFinish) { | ||
| if (timeout != null && timeout > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (timeout != null && timeout > 0)
checkpointfut.futureFor(CheckpointState.FINISHED).get(timeout, TimeUnit.MILLISECONDS);
else
checkpointfut.futureFor(CheckpointState.FINISHED).get();
| } | ||
| return "Checkpoint completed on node: " + ignite.localNode().id(); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else
return "Checkpoint triggered on node: " + ignite.localNode().id();
|
|
||
| /** {@inheritDoc} */ | ||
| @Override protected String run(@Nullable CheckpointCommandArg arg) throws IgniteException { | ||
| if (!CU.isPersistenceEnabled(ignite.configuration())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One line statements written without quote.
Please, take a look at style guide:
https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines
if (!CU.isPersistenceEnabled(ignite.configuration()))
throw new IgniteException("Can't checkpoint on in-memory node");
| throw new IgniteException("Can't checkpoint on in-memory node"); | ||
| } | ||
|
|
||
| String reason = arg != null && arg.reason() != null ? arg.reason() : "control.sh"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's skip arg null checks here and in other places.
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override protected String run(@Nullable CheckpointCommandArg arg) throws IgniteException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove Nullable annotation.
| stopAllGrids(); | ||
| cleanPersistenceDir(); | ||
| injectTestSystemOut(); | ||
| super.beforeTest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super.beforeTest(); must be invoked first.
| @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception { | ||
| IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName); | ||
|
|
||
| if (clusterState == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use
GridCommandHandlerAbstractTest#persistenceEnable instead of clusterState flag
|
|
||
| import java.util.Objects; | ||
| import java.util.regex.Pattern; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| /** Test for checkpoint in control.sh command. */ | ||
| public class GridCommandHandlerCheckpointTest extends GridCommandHandlerAbstractTest { | ||
| /** */ | ||
| protected final ListeningTestLogger listeningLog = new ListeningTestLogger(log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| IgniteEx srv = startGrids(2); | ||
| IgniteEx cli = startClientGrid("client"); | ||
|
|
||
| LogListener checkpointFinishedLsnr = LogListener.matches("Checkpoint finished").build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid duplication of creating checkpointFinishedLsnr in each test, let's create a field and reuse it across all tests:
/** */
private final ListeningTestLogger listeningLog = new ListeningTestLogger(log);
/** */
private final LogListener checkpointFinishedLsnr = LogListener.matches("Checkpoint finished").build();
/** */
@Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception {
IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName);
if (!persistenceEnable())
cfg.setDataStorageConfiguration(null);
listeningLog.registerListener(checkpointFinishedLsnr);
cfg.setGridLogger(listeningLog);
return cfg;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| srv.createCache("testCache"); | ||
| assertEquals(EXIT_CODE_UNEXPECTED_ERROR, execute("--checkpoint")); | ||
|
|
||
| String out = testOut.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move it after the line outputContains(...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public @Nullable Collection<ClusterNode> nodes( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's place on a single line.
@Override public @Nullable Collection<ClusterNode> nodes(Collection<ClusterNode> nodes, CheckpointCommandArg arg) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
|
||
| /** */ | ||
| @Argument(description = "Timeout in milliseconds", optional = true) | ||
| private Long timeout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could use a primitive type?
private long timeout = -1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override protected String run(CheckpointCommandArg arg) throws IgniteException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest refactoring slightly to make the code easier to read:
/** {@inheritDoc} */
@Override protected String run(CheckpointCommandArg arg) throws IgniteException {
if (!CU.isPersistenceEnabled(ignite.configuration()))
throw new IgniteException("Can't checkpoint on in-memory node");
try {
GridCacheDatabaseSharedManager dbMgr = (GridCacheDatabaseSharedManager)ignite.context().cache().context().database();
CheckpointProgress checkpointfut = dbMgr.forceCheckpoint(arg.reason());
if (!arg.waitForFinish())
return "Checkpoint triggered on node: " + ignite.localNode().id();
long timeout = arg.timeout();
if (timeout > 0)
checkpointfut.futureFor(CheckpointState.FINISHED).get(timeout, TimeUnit.MILLISECONDS);
else
checkpointfut.futureFor(CheckpointState.FINISHED).get();
return "Checkpoint completed on node: " + ignite.localNode().id();
}
catch (Exception e) {
throw new IgniteException("Failed to force checkpoint on node: " + ignite.localNode().id(), e);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+, thank you for refactoring
| @Override protected void writeExternalData(ObjectOutput out) throws IOException { | ||
| U.writeString(out, reason); | ||
| out.writeBoolean(waitForFinish); | ||
| out.writeObject(timeout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out.writeLong
| /** {@inheritDoc} */ | ||
| @Override protected String run(CheckpointCommandArg arg) throws IgniteException { | ||
| if (!CU.isPersistenceEnabled(ignite.configuration())) | ||
| return ignite.localNode().id() + ": PDS disabled, checkpoint skipped"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PDS -> Persistence
| else | ||
| checkpointfut.futureFor(CheckpointState.FINISHED).get(); | ||
| return ignite.localNode().id() + ": Checkpoint finished"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line after
| if (timeout > 0) | ||
| checkpointfut.futureFor(CheckpointState.FINISHED).get(timeout, TimeUnit.MILLISECONDS); | ||
| else | ||
| checkpointfut.futureFor(CheckpointState.FINISHED).get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line after
modules/core/src/main/java/org/apache/ignite/internal/management/checkpoint/CheckpointTask.java
Outdated
Show resolved
Hide resolved
| DataStorageConfiguration storageCfg = new DataStorageConfiguration(); | ||
|
|
||
| DataRegionConfiguration dfltRegionCfg = new DataRegionConfiguration(); | ||
| dfltRegionCfg.setName("default_in_memory_region"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be combined like this:
storageCfg.setDefaultDataRegionConfiguration(new DataRegionConfiguration()
.setName("default_in_memory_region")
.setPersistenceEnabled(false));
```
| storageCfg.setDefaultDataRegionConfiguration(dfltRegionCfg); | ||
|
|
||
| if (igniteInstanceName.contains("persistent_instance")) { | ||
| DataRegionConfiguration persistentRegionCfg = new DataRegionConfiguration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be inlined in setDataRegionConfiguration
| @Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception { | ||
| IgniteConfiguration cfg = super.getConfiguration(igniteInstanceName); | ||
|
|
||
| if (!persistenceEnable()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, move this below to keep all storage configuration in one place
| persistenceEnable(true); | ||
|
|
||
| IgniteEx srv = startGrids(2); | ||
| IgniteEx cli = startClientGrid("client"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
|
|
||
| assertEquals(EXIT_CODE_OK, execute("--checkpoint", "--reason", "test_reason")); | ||
|
|
||
| LogListener checkpointReasonLsnr = LogListener.matches("reason='test_reason'").build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
|
|
||
| assertTrue(GridTestUtils.waitForCondition(checkpointFinishedLsnr::check, 10_000)); | ||
| assertTrue(GridTestUtils.waitForCondition(checkpointReasonLsnr::check, 10_000)); | ||
| assertFalse(testOut.toString().contains("PDS disabled")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| outputContains(": Checkpoint started"); | ||
|
|
||
| testOut.reset(); | ||
| checkpointFinishedLsnr.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| public void testCheckpointInMemoryCluster() throws Exception { | ||
| persistenceEnable(false); | ||
|
|
||
| IgniteEx srv = startGrids(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| persistenceEnable(false); | ||
|
|
||
| IgniteEx srv = startGrids(2); | ||
| startClientGrid("client"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| cacheCli.put(3, 3); | ||
|
|
||
| assertEquals(EXIT_CODE_OK, execute("--checkpoint", "--wait-for-finish")); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no new line
| cacheCli.put(1, 1); | ||
|
|
||
| assertEquals(EXIT_CODE_OK, execute("--checkpoint")); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no new line
| assertEquals(EXIT_CODE_OK, execute("--checkpoint")); | ||
|
|
||
| assertTrue(GridTestUtils.waitForCondition(checkpointFinishedLsnr::check, 10_000)); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no new line
|
|
||
| assertTrue(GridTestUtils.waitForCondition(checkpointFinishedLsnr::check, 10_000)); | ||
|
|
||
| assertFalse(testOut.toString().contains("PDS disabled")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| assertFalse(testOut.toString().contains("PDS disabled")); | ||
| outputContains(": Checkpoint started"); | ||
|
|
||
| testOut.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| startClientGrid("client"); | ||
| srv.cluster().state(ClusterState.ACTIVE); | ||
|
|
||
| srv.createCache("testCache"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
|
|
||
| srv.createCache("testCache"); | ||
| assertEquals(EXIT_CODE_OK, execute("--checkpoint")); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no new line
| public void testCheckpointTimeout() throws Exception { | ||
| persistenceEnable(true); | ||
|
|
||
| IgniteEx srv = startGrids(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| public void testMixedCluster() throws Exception { | ||
| mixedConfig = true; | ||
|
|
||
| IgniteEx node0 = startGrid("in-memory_instance"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
| DataRegionConfiguration[] node1Regions = node1Storage.getDataRegionConfigurations(); | ||
| assertEquals(1, node1Regions.length); | ||
|
|
||
| DataRegionConfiguration persistentRegion = node1Regions[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
|
|
||
| assertEquals(EXIT_CODE_OK, execute("--checkpoint", "--wait-for-finish")); | ||
|
|
||
| assertTrue(checkpointFinishedLsnr.check()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line after
|
|
||
| if (mixedConfig) { | ||
| DataStorageConfiguration storageCfg = new DataStorageConfiguration(); | ||
| DataRegionConfiguration dfltRegionCfg = new DataRegionConfiguration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single usage.
Let's inline dfltDataRegion:
storageCfg.setDefaultDataRegionConfiguration(new DataRegionConfiguration().setName("default_in_memory_region").setPersistenceEnabled(false));
| private final LogListener checkpointFinishedLsnr = LogListener.matches("Checkpoint finished").build(); | ||
|
|
||
| /** */ | ||
| private boolean mixedConfig = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useless initialization. Default value false, already.
| private boolean mixedConfig = false; | ||
|
|
||
| /** */ | ||
| private static final String persistentRegionName = "pds-reg"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constants must be in UPPER_CASE.
Please, move constant to the top of class.
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
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.