You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/09/11 16:28:09 UTC
[GitHub] [geode] mivanac opened a new pull request, #7851: GEODE-10419: initial commit
mivanac opened a new pull request, #7851:
URL: https://github.com/apache/geode/pull/7851
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac merged pull request #7851: GEODE-10419: Enhancment of backup disk-store command
Posted by GitBox <gi...@apache.org>.
mivanac merged PR #7851:
URL: https://github.com/apache/geode/pull/7851
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a diff in pull request #7851: GEODE-10419: Enhancment of backup disk-store command
Posted by GitBox <gi...@apache.org>.
albertogpz commented on code in PR #7851:
URL: https://github.com/apache/geode/pull/7851#discussion_r970679236
##########
geode-docs/tools_modules/gfsh/command-pages/backup.html.md.erb:
##########
@@ -59,6 +59,11 @@ backup disk-store --dir=value [--baseline-dir=value]
<td>Directory that contains the baseline backup used for comparison during an incremental backup.
<p>An incremental backup operation backs up any data that is not present in the directory specified in <span class="keyword parmname">‑‑baseline-dir</span>. If the member cannot find previously backed up data or if the previously backed up data is corrupt, the command performs a full backup on that member.</p></td>
</tr>
+<tr>
+<td><span class="keyword parmname">‑‑include-disk-stores</span></td>
+<td>List of disk-stores to include in backup.
+<p>Selective backup of disk-stores listed in <span class="keyword parmname">‑‑include-disk-stores</span>. Specified disk-stores must exist in system. If parameter is not specified, all disk-stores are backup.</p></td>
Review Comment:
Any reason for not adding "this" in "If this parameter is not specified..."?
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #7851: GEODE-10419: initial commit
Posted by GitBox <gi...@apache.org>.
mivanac commented on PR #7851:
URL: https://github.com/apache/geode/pull/7851#issuecomment-1246541895
RFC is linked in ticket, but I can also add description in PR.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a diff in pull request #7851: GEODE-10419: Enhancment of backup disk-store command
Posted by GitBox <gi...@apache.org>.
mivanac commented on code in PR #7851:
URL: https://github.com/apache/geode/pull/7851#discussion_r970681780
##########
geode-docs/tools_modules/gfsh/command-pages/backup.html.md.erb:
##########
@@ -59,6 +59,11 @@ backup disk-store --dir=value [--baseline-dir=value]
<td>Directory that contains the baseline backup used for comparison during an incremental backup.
<p>An incremental backup operation backs up any data that is not present in the directory specified in <span class="keyword parmname">‑‑baseline-dir</span>. If the member cannot find previously backed up data or if the previously backed up data is corrupt, the command performs a full backup on that member.</p></td>
</tr>
+<tr>
+<td><span class="keyword parmname">‑‑include-disk-stores</span></td>
+<td>List of disk-stores to include in backup.
+<p>Selective backup of disk-stores listed in <span class="keyword parmname">‑‑include-disk-stores</span>. Specified disk-stores must exist in system. If parameter is not specified, all disk-stores are backup.</p></td>
Review Comment:
I skipped that part. I will update.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on pull request #7851: GEODE-10419: initial commit
Posted by GitBox <gi...@apache.org>.
albertogpz commented on PR #7851:
URL: https://github.com/apache/geode/pull/7851#issuecomment-1246547375
> RFC is linked in ticket, but I can also add description in PR.
Yes, looking at the ticket you reach the RFC. But it would be nice to know from the title of the PR what this PR is about.
The description could be optional.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a diff in pull request #7851: GEODE-10419: Enhancment of backup disk-store command
Posted by GitBox <gi...@apache.org>.
mivanac commented on code in PR #7851:
URL: https://github.com/apache/geode/pull/7851#discussion_r970661703
##########
geode-docs/tools_modules/gfsh/command-pages/backup.html.md.erb:
##########
@@ -59,6 +59,11 @@ backup disk-store --dir=value [--baseline-dir=value]
<td>Directory that contains the baseline backup used for comparison during an incremental backup.
<p>An incremental backup operation backs up any data that is not present in the directory specified in <span class="keyword parmname">‑‑baseline-dir</span>. If the member cannot find previously backed up data or if the previously backed up data is corrupt, the command performs a full backup on that member.</p></td>
</tr>
+<tr>
+<td><span class="keyword parmname">‑‑include-disk-stores</span></td>
+<td>List of disk-stores to include in backup.
+<p>Selective backup of disk-stores listed in <span class="keyword parmname">‑‑include-disk-stores</span>. Specified disk-stores must exist in system. If parameter is not specified, all disk-stores are backup.</p></td>
Review Comment:
Thanks for comments. Updated.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jvarenina commented on a diff in pull request #7851: GEODE-10419: Enhancment of backup disk-store command
Posted by GitBox <gi...@apache.org>.
jvarenina commented on code in PR #7851:
URL: https://github.com/apache/geode/pull/7851#discussion_r970802822
##########
geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupTask.java:
##########
@@ -38,6 +42,7 @@
class BackupTask {
private static final Logger logger = LogService.getLogger();
+ private static final String CLUSTER_CONFIG_DISK_STORE_NAME = "cluster_config";
Review Comment:
Just a minor comment. Maybe to change the access modifier of "InternalConfigurationPersistenceService.CLUSTER_CONFIG_DISK_STORE_NAME" to public and use it instead of this one to avoid duplicated code.
No further comments from my side, everything else looks good. Good work!
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a diff in pull request #7851: GEODE-10419: Enhancment of backup disk-store command
Posted by GitBox <gi...@apache.org>.
albertogpz commented on code in PR #7851:
URL: https://github.com/apache/geode/pull/7851#discussion_r970619899
##########
geode-docs/tools_modules/gfsh/command-pages/backup.html.md.erb:
##########
@@ -59,6 +59,11 @@ backup disk-store --dir=value [--baseline-dir=value]
<td>Directory that contains the baseline backup used for comparison during an incremental backup.
<p>An incremental backup operation backs up any data that is not present in the directory specified in <span class="keyword parmname">‑‑baseline-dir</span>. If the member cannot find previously backed up data or if the previously backed up data is corrupt, the command performs a full backup on that member.</p></td>
</tr>
+<tr>
+<td><span class="keyword parmname">‑‑include-disk-stores</span></td>
+<td>List of disk-stores to include in backup.
+<p>Selective backup of disk-stores listed in <span class="keyword parmname">‑‑include-disk-stores</span>. Specified disk-stores must exist in system. If parameter is not specified, all disk-stores are backup.</p></td>
Review Comment:
I suggest the following changes to the phrasing:
The specified disk-stores must exist in the system.
If this parameter is not specified, all disk-stores are included in the backup.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] mivanac commented on a diff in pull request #7851: GEODE-10419: Enhancment of backup disk-store command
Posted by GitBox <gi...@apache.org>.
mivanac commented on code in PR #7851:
URL: https://github.com/apache/geode/pull/7851#discussion_r970804245
##########
geode-core/src/main/java/org/apache/geode/internal/cache/backup/BackupTask.java:
##########
@@ -38,6 +42,7 @@
class BackupTask {
private static final Logger logger = LogService.getLogger();
+ private static final String CLUSTER_CONFIG_DISK_STORE_NAME = "cluster_config";
Review Comment:
Thanks. Updated.
--
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@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org