You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/07/27 22:55:21 UTC
[GitHub] [accumulo] Manno15 opened a new pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Manno15 opened a new pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215
Utilizes thrift to remove any use of SiteConfiguration from `FateCommand`.
WIP as I am still looking into any possible refactors and improvements to the code but any feedback is appreciated. I left a comment inside AdminUtil as I believe additional work can be done there to improve exception handling that coincides with the changes done here to `FateCommand`. There is also a TODO there to remove the use of System.exit.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1079720002
This should mostly be ready for review. ~~I confirmed fail, delete, list, print, and dump work as expected. I am not quite getting cancel-submitted to work at the moment. @milleruntime~~ I confirmed that each command opt works as expected.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r678651686
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
Review comment:
Done in my latest commit
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r679258824
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
Review comment:
> Instead of returning "String" with "Print" in the method name, this could return a map of fate IDs to statuses, or some similar data structure.
Is there a benefit you had in mind with this possible change? Currently, the returning string is the entire output from `Admin.print` that then gets printed to the shell. Admin print handles all the formatting, the only thing FateCommand has to do is print it to the shell.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1079720002
This should mostly be ready for review. I confirmed fail, delete, list, print, and dump work as expected. I am not quite getting cancele-submitted to work at the moment.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1059327344
These changes look pretty good and I think this would be nice to get into 2.1. @Manno15 what kind of testing were you able to do? My only concern is leaking the FATE types in `TransactionStatus`. We do blind casting of the serialized FATE repos on the server side so I think it might be possible to return some of the types not in the API.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r678574769
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
+ String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fateDump(List<String> args, String secretOption) throws AccumuloException;
Review comment:
Dump provides a lot more information, I will post a snippet below
<details>
<pre>
root@uno> fate dump
[
{
"txid": "58136bf11a1bc7d5",
"stack": [
{
"org.apache.accumulo.manager.tableOps.TraceRepo": {
"traceId": 0,
"parentId": 0,
"repo": {
"org.apache.accumulo.manager.tableOps.create.CreateTable": {
"tableInfo": {
"tableName": "test",
"namespaceId": {
"canonical": "+default"
},
"timeType": "MILLIS",
"user": "root",
"initialTableState": "ONLINE",
"initialSplitSize": 19,
"splitFile": "hdfs://groot:8020/accumulo/tmp/fate-58136bf11a1bc7d5/splits",
"splitDirsFile": "hdfs://groot:8020/accumulo/tmp/fate-58136bf11a1bc7d5/splitsDirs",
"props": {
"table.majc.compaction.strategy.opts.large.compress.threshold": "100M",
"table.iterator.majc.vers": "20,org.apache.accumulo.core.iterators.user.VersioningIterator",
"table.constraint.1": "org.apache.accumulo.core.data.constraints.DefaultKeySizeConstraint",
"table.iterator.scan.vers.opt.maxVersions": "1",
"table.iterator.minc.vers": "20,org.apache.accumulo.core.iterators.user.VersioningIterator",
"table.majc.compaction.strategy.opts.large.compress.type": "gz",
"table.iterator.majc.vers.opt.maxVersions": "1",
"table.majc.compaction.strategy": "org.apache.accumulo.tserver.compaction.strategies.BasicCompactionStrategy",
"table.iterator.minc.vers.opt.maxVersions": "1",
"table.iterator.scan.vers": "20,org.apache.accumulo.core.iterators.user.VersioningIterator",
"table.majc.compaction.strategy.opts.filter.size": "250M"
}
}
}
}
}
}
]
</pre>
</details>
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1059341120
While testing with Uno and the `SlowIterator`, I noticed that multiple FATE Txs get kicked off if you issue multiple compact commands. This may be unrelated to your changes but I think it is noteworthy. I thought there were checks that prevent another compaction from running if the table doesn't need to be compacted. It could be that the check for this happens in CompactRange, after the FATE Tx gets started.
<pre>
root@uno> fate print
txid: 1bc091ffb9162774 status: IN_PROGRESS op: CompactRange locked: [R:+default, R:1] locking: [] top: CompactionDriver created: 2022-03-04T16:38:44.088Z
txid: 543834dd76654bc1 status: IN_PROGRESS op: CompactRange locked: [R:+default, R:1] locking: [] top: CompactionDriver created: 2022-03-04T16:56:29.574Z
txid: 7e20a7508cac33d9 status: IN_PROGRESS op: CompactRange locked: [R:+default, R:1] locking: [] top: CompactionDriver created: 2022-03-04T16:57:25.536Z
txid: 1ca0ac1b2198cfe0 status: IN_PROGRESS op: CompactRange locked: [R:+default, R:1] locking: [] top: CompactionDriver created: 2022-03-04T16:38:17.825Z
4 transactions
root@uno> listcompactions
SERVER | AGE | TYPE | REASON | READ | WROTE | TABLE | TABLET | INPUT | OUTPUT | ITERATORS | ITERATOR OPTIONS
ip-10-113-12-25:10000 | 19m15s | FULL | USER | 224K | 224K | ci | 1<< | 3 | /1/default_tablet/A000005e.rf_tmp | [] | {}
</pre>
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1059329543
It has been awhile, so I do not fully recall. I will look into testing it
again.
On Fri, Mar 4, 2022 at 11:42 AM Mike Miller ***@***.***>
wrote:
> These changes look pretty good and I think this would be nice to get into
> 2.1. @Manno15 <https://github.com/Manno15> what kind of testing were you
> able to do? My only concern is leaking the FATE types in TransactionStatus.
> We do blind casting of the serialized FATE repos on the server side so I
> think it might be possible to return some of the types not in the API.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/2215#issuecomment-1059327344>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHASSV2LLPIDN53L7EWPDVDU6I4QZANCNFSM5BDFUX4Q>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r819807671
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -150,6 +150,37 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Fails a fate transaction based on the given txID. At least one txID must be provided.
+ *
+ * @param txids
+ * Transaction IDs to fail.
+ * @since 2.1.0
+ */
+ void fateFail(List<String> txids) throws AccumuloException;
+
+ /**
+ * Deletes a fate transaction based on the given txID. At least one txID must be provided.
+ *
+ * @param txids
+ * Transaction IDs to delete.
+ * @since 2.1.0
+ */
+ void fateDelete(List<String> txids) throws AccumuloException;
+
+ /**
+ * Gathers Transaction status information for either all fate transactions or requested txIDs.
+ *
+ * @param txids
+ * Transaction IDs to use as a filter. Optional.
+ * @param tStatus
+ * Parsed TStatus for print filter. Optional.
+ * @return A list of TransactionStatues for corresponding txids
+ * @since 2.1.0
+ */
+ List<TransactionStatus> fateStatus(List<String> txids, List<String> tStatus)
+ throws AccumuloException;
+
Review comment:
I think these are good so far but we need a way to get the `TxIds`. We need a method like this on InstanceOperations:
`List<TransactionStatus> fatePrint() throws AccumuloException;`
Here is an example of how it could be used:
<pre>
var list = client.instanceOperations().fatePrint();
for (var ts : list) {
client.instanceOperations().fateFail(List.of(ts.getTxid()));
}
</pre>
I also think this would be a good opportunity to use a stronger type for Txid. This was discussed recently here: https://github.com/apache/accumulo/issues/2473#issuecomment-1039473784
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-930247672
The latest commit includes the upgrade to thrift 0.15.0
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1064134839
> The latest commit fixed the new merge conflicts. This PR is now more a WIP. Several changes to FateCommand has happened and this will need to be further tested to make sure everything still works. Also, the `FateCommandTest` needs further changes due to the API changes and removal of `SiteConfiguration` from FateCommand. It might make sense for the test to be moved to an IT. Thoughts on the test @milleruntime?
Possibly. But I just created `FateCommandTest` to have as a unit test since we had nothing before. I have more changes with my refactor in https://github.com/apache/accumulo/pull/2475
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-887926761
These are general questions - rather than comment on the code that triggered the question for me, it seems better to just raise them in general.
Would it be possible to replace passing the secret and instead use the authentication of the user that launched the shell? Depending on granted user permissions, it may be desirable to permit certain FATE command options like print and dump to a wider group than fail or delete. FATE fail or delete should require "root" access, but other operators would have valid use of print or dump to examine system state. This would enable more restrictive access policies for operations that change system state versus operations that examine the system state on a users granted permissions rather than needing the secret. One down side of this approach could what is needed to perform the authentication - FATE fail requires the global lock which implies that a manager processes is not going to be available.
If the client authorization cannot be used, wondering if it would be better to use char[] or a byte array instead of a string? Even better would be if the hashed digest value derived from the secret was used in the methods. If this is mirroring other places in the code, then maybe as a follow on issue as a general improvement in password best practices.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r681739859
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
Review comment:
Okay, that makes a lot of sense. My viewpoint was too narrowly focused.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1073011230
@dlmarion I merged in your changes in #2575. Let me know I missed anything. I am still working on adapting the tests from the previous changes as well.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1064115545
The latest commit fixed the new merge conflicts. This PR is now more a WIP. Several changes to FateCommand has happened and this will need to be further tested to make sure everything still works. Also, the `FateCommandTest` needs further changes due to the API changes and removal of `SiteConfiguration` from FateCommand. It might make sense for the test to be moved to an IT. Thoughts on the test @milleruntime?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-894377704
Converting to draft PR as the most recent changes are still a WIP. I would still appreciate some feedback. Specifically with moving `TransactionStatus` to public API.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r678581217
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
Review comment:
> Not sure from this what values are appropriate for "args". Are these just a list of FaTE transaction IDs to fail? The parameters to these methods could be more specific.
Yes, Args will be the TiDs. Initially I was going to have it be more broad and include other things inside it. I can see how it is confusing now. Since I have parameters specifically for FilterTiDs for the print statement I could possibly combine the two parameters.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r678576722
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
+ String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fateDump(List<String> args, String secretOption) throws AccumuloException;
Review comment:
Whether or not we want all that information I guess is up for debate.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r681389046
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
Review comment:
> > Instead of returning "String" with "Print" in the method name, this could return a map of fate IDs to statuses, or some similar data structure.
>
> Is there a benefit you had in mind with this possible change?
Well, mainly just that APIs should return objects that can be manipulated/read/transformed by calling code. It's not a console REPL where we have only STDIN/STDOUT and have to parse STDOUT from a command. We can have actual typed data. So, there's no reason to resort to "String" types to represent complex information in the API.
> Currently, the returning string is the entire output from `Admin.print` that then gets printed to the shell. Admin print handles all the formatting, the only thing FateCommand has to do is print it to the shell.
Right, that's all that the shell needs to do... but if we're going through the effort of creating a public API endpoint, we're not doing it just for the shell. The shell can take a complex object returned from the public API and format it into a string for printing. But other callers to this API endpoint might want to do something different with the data. When creating a new public API endpoint, we should try to consider the value it has to a generic caller of the public API, and not just the single use case that inspired its creation.
> Instead of 'Print', I could change the name to "fateStatus" as I think that more accurately describes the intended use of the function. It shows the status of either all the txid's in operation or the specific one the user requests.
The name is fine. It could return something that looks like `Set<FateStatus>` or `Map<Txid,Status>` or similar.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1085452938
> I think this looks good. @ctubbsii did you want to give it another look before merging?
I would like to take a look, but I might not have time until next week.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r677880078
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
Review comment:
Wondering if it would be better to use char[] or a byte array instead of a string? Even better would be if the hashed value was used. If this is mirroring other places in the code, then maybe as a follow on issue as a general improvement in password best practices.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-930247672
The latest commit includes the upgrade to thrift 0.15.0
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r678564250
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
Review comment:
We wouldn't want TStatus in the public API, but this could be a set of strings for the various statuses. The implementation can convert to the TStatus type, if necessary.
Instead of returning "String" with "Print" in the method name, this could return a map of fate IDs to statuses, or some similar data structure.
##########
File path: core/src/main/thrift/client.thrift
##########
@@ -331,6 +338,18 @@ service ClientService {
1:ThriftTableOperationException tope
)
+ string executeAdminOperation(
+ 3:trace.TInfo tinfo
+ 4:security.TCredentials credentials
+ 2:AdminOperation op
+ 5:list<string> arguments
+ 6:set<i64> filtertxids
+ 7:list<string> filterStatues
+ 8:string secretOption
Review comment:
For a new method that doesn't need to preserve backwards compatibility, the parameters can be numbered in order starting at 1.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
+ String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fateDump(List<String> args, String secretOption) throws AccumuloException;
Review comment:
Not sure if we need both "Dump" and "Print" methods. They seem to be doing similar things.
##########
File path: core/src/main/java/org/apache/accumulo/fate/AdminUtil.java
##########
@@ -533,26 +533,29 @@ public void deleteLocks(ZooReaderWriter zk, String path, String txidStr)
}
}
+ // Follow on ticket to rework exception handling to
+ // coincide with changes to FateCommand. At the moment, user has no feedback of why something
+ // failed without having to go check server logs.
@SuppressFBWarnings(value = "DM_EXIT",
justification = "TODO - should probably avoid System.exit here; "
+ "this code is used by the fate admin shell command")
public boolean checkGlobalLock(ZooReaderWriter zk, ServiceLockPath path) {
try {
if (ServiceLock.getLockData(zk.getZooKeeper(), path) != null) {
- System.err.println("ERROR: Manager lock is held, not running");
+ log.error("ERROR: Manager lock is held, not running");
Review comment:
If changing these to log messages, including "ERROR" in the message is redundant.
##########
File path: core/src/main/java/org/apache/accumulo/fate/AdminUtil.java
##########
@@ -533,26 +533,29 @@ public void deleteLocks(ZooReaderWriter zk, String path, String txidStr)
}
}
+ // Follow on ticket to rework exception handling to
+ // coincide with changes to FateCommand. At the moment, user has no feedback of why something
+ // failed without having to go check server logs.
@SuppressFBWarnings(value = "DM_EXIT",
justification = "TODO - should probably avoid System.exit here; "
+ "this code is used by the fate admin shell command")
public boolean checkGlobalLock(ZooReaderWriter zk, ServiceLockPath path) {
try {
if (ServiceLock.getLockData(zk.getZooKeeper(), path) != null) {
- System.err.println("ERROR: Manager lock is held, not running");
+ log.error("ERROR: Manager lock is held, not running");
if (this.exitOnError)
System.exit(1);
else
return false;
}
} catch (KeeperException e) {
- System.err.println("ERROR: Could not read manager lock, not running " + e.getMessage());
+ log.error("ERROR: Could not read manager lock, not running " + e.getMessage());
if (this.exitOnError)
System.exit(1);
else
return false;
} catch (InterruptedException e) {
- System.err.println("ERROR: Could not read manager lock, not running" + e.getMessage());
+ log.error("ERROR: Could not read manager lock, not running" + e.getMessage());
Review comment:
To include the stack trace in the log message:
```suggestion
log.error("Could not read manager lock, not running", e);
```
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
Review comment:
I think I agree with @EdColeman 's observation that you can probably just require these to be used with an authenticated user with Admin permission, rather than provide the instance secret. The server side can use the instance secret from its config file, without needing to pass it.
Not sure from this what values are appropriate for "args". Are these just a list of FaTE transaction IDs to fail? The parameters to these methods could be more specific.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r681740810
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +150,46 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
Review comment:
This was just a mistake by me. I pushed those up without editing the description of them.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-888389483
@EdColeman. I was following what was already in Fate Command at the time. It allowed the user to pass in a secret instead of using what was in the SiteConfiguration. That is not the default case and only occurs if a user specifies that parameter. I am not quite sure on the initial goal of that though so maybe it is possible to use user authorization but I have not taken a look into it. As for object type, I could replace it with a byte array but the object is only used to create a new `ZooReaderWriter` which takes in a string so I am not sure the gain there.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1079720002
This should mostly be ready for review. I confirmed fail, delete, list, print, and dump work as expected. I am not quite getting cancel-submitted to work at the moment. @milleruntime
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r677880078
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
Review comment:
Wondering if it would be better to use char[] or a byte array instead of a string? Even better would be if the hashed value was used. If this is mirroring other places in the code, then maybe as a follow on issue as a general improvement in password best practices.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-894377704
~~Converting to draft PR as the most recent changes are still a WIP. I would still appreciate some feedback.~~ Specifically with moving `TransactionStatus` to public API.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r681391239
##########
File path: core/src/main/thrift/client.thrift
##########
@@ -331,6 +338,16 @@ service ClientService {
1:ThriftTableOperationException tope
)
+ string executeAdminOperation(
+ 2:trace.TInfo tinfo
Review comment:
This can be numbered starting at 1. The numbers for the throws exceptions can also start at 1. They are a separate context, and it doesn't matter if they reuse numbers across contexts, as long as the numbers aren't reused within the same parameter/exception context.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +150,46 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
Review comment:
These javadocs should say what the method does normally. The explanation for when an exception is thrown can be in a javadoc `@throws SomeException when <explanation here>` line.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +150,46 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param txids
+ * Transaction IDs.
+ * @since 2.1.0
+ */
+ void fateFail(List<String> txids) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param txids
+ * Transaction IDs.
+ * @since 2.1.0
+ */
+ void fateDelete(List<String> txids) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param txids
+ * Transaction IDs.
+ * @param tStatus
+ * Parsed TStatus for print filter.
+ * @return String containing the output to print to the shell.
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> txids, List<String> tStatus) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param txids
+ * Transaction IDs.
+ * @return String containing the output to print to the shell.
+ * @since 2.1.0
+ */
+ String fateDump(List<String> txids) throws AccumuloException;
Review comment:
See previous review conversation for discussion about consolidating these, and having a meaningful return type other than String. These APIs should not be written exclusively for use with the Shell, but should have meaningful return types of their own, and not just formatted strings for the shell.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r678635260
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
+ String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fateDump(List<String> args, String secretOption) throws AccumuloException;
Review comment:
If we had one method that returned the set of transaction IDs, their states, and their blobs like this, code that calls this method can decide what pieces they care about.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1064125492
The build errors are because I have parts of the test commented out for the moment.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-1064115545
The latest commit fixed the new merge conflicts. This PR is now more a WIP. Several changes to FateCommand has happened and this will need to be further tested to make sure everything still works. Also, the `FateCommandTest` needs further changes due to the API changes and removal of `SiteConfiguration` from FateCommand. It might make sense for the test to be moved to an IT, such as `FateIT`. Thoughts on the test @milleruntime?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r681114518
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
Review comment:
Instead of 'Print', I could change the name to "fateStatus" as I think that more accurately describes the intended use of the function. It shows the status of either all the txid's in operation or the specific one the user requests.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2215: WIP: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r679258824
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -149,6 +152,59 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateFail(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ boolean fateDelete(List<String> args, String secretOption) throws AccumuloException;
+
+ /**
+ * Throws an exception if a tablet server can not be contacted.
+ *
+ * @param args
+ * Command line arguments passed in from user command.
+ * @param filterTxid
+ * Parsed transaction IDs for print filter.
+ * @param filterStatus
+ * Parsed TStatus for print filter.
+ * @param secretOption
+ * Specified instance secret to use for commands.
+ *
+ * @since 2.1.0
+ */
+ String fatePrint(List<String> args, Set<Long> filterTxid, EnumSet<TStatus> filterStatus,
Review comment:
> Instead of returning "String" with "Print" in the method name, this could return a map of fate IDs to statuses, or some similar data structure.
Is there a benefit you had in mind with this possible change? Currently, the returning string is the entire output from `Admin.print` that then gets printed to the shell. Admin print handles all the formatting, the only thing FateCommand has to do is print it to the shell.
For reference, the current output looks something like this :
<details>
</details>
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#issuecomment-894477962
I have finalized the consolidation of the fate print and dump options. Now, the stack info gets stored inside `TransactionStatus` as well. The only part I am unsure of is creating the gson object inside adminUtil but I couldn't find any alternatives.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2215: Remove the use of SiteConfiguration from FateCommand
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2215:
URL: https://github.com/apache/accumulo/pull/2215#discussion_r819818926
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java
##########
@@ -150,6 +150,37 @@ boolean testClassLoad(final String className, final String asTypeName)
*/
void waitForBalance() throws AccumuloException;
+ /**
+ * Fails a fate transaction based on the given txID. At least one txID must be provided.
+ *
+ * @param txids
+ * Transaction IDs to fail.
+ * @since 2.1.0
+ */
+ void fateFail(List<String> txids) throws AccumuloException;
+
+ /**
+ * Deletes a fate transaction based on the given txID. At least one txID must be provided.
+ *
+ * @param txids
+ * Transaction IDs to delete.
+ * @since 2.1.0
+ */
+ void fateDelete(List<String> txids) throws AccumuloException;
+
+ /**
+ * Gathers Transaction status information for either all fate transactions or requested txIDs.
+ *
+ * @param txids
+ * Transaction IDs to use as a filter. Optional.
+ * @param tStatus
+ * Parsed TStatus for print filter. Optional.
+ * @return A list of TransactionStatues for corresponding txids
+ * @since 2.1.0
+ */
+ List<TransactionStatus> fateStatus(List<String> txids, List<String> tStatus)
+ throws AccumuloException;
+
Review comment:
If this would benefit from some of the more aggressive FaTE refactoring you're working on that might go in 3.0, I'd be okay with pushing this PR off until then. But, otherwise, I'd generally agree with your comment above that this would be good to get into 2.1 (based on the intended outcome, not the code itself, since I haven't yet had a chance to review the code).
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org