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 2022/06/15 17:44:29 UTC
[GitHub] [accumulo] milleruntime opened a new pull request, #2780: New FateOperations API
milleruntime opened a new pull request, #2780:
URL: https://github.com/apache/accumulo/pull/2780
Changes taken from https://github.com/apache/accumulo/pull/2215 with some improvements. I added a `FateOperations` interface instead of having the methods hang off of `InstanceOperations`. I also created `FateTransactionStatus` interface to be a part of the API. I could add more methods to `FateOperations` but wasn't sure if that was the way to go or if the methods should be in `InstanceOperations`
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1235793511
I started work to move the new FateCommand options (cancel and summary) added in 2.1 to the Admin command in https://github.com/apache/accumulo/pull/2914
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r898721597
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateOperations.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+import java.util.Set;
+
+import org.apache.accumulo.core.client.AccumuloException;
+
+public interface FateOperations {
+
+ /**
+ * 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(Set<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(Set<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 set of TransactionStatues for corresponding txids
+ * @since 2.1.0
+ */
+ List<FateTransactionStatus> fateStatus(Set<String> txids, List<String> tStatus)
+ throws AccumuloException;
Review Comment:
If you had methods, `fail()` and `delete()` on `FateTransactionStatus`, this list (or set, really.. since the items should be unique) is all you'd really need, and you wouldn't need the separate `FateOperations` interface. It could also be a stream, so you can do things like:
```java
client.instanceOperations().fateTransactions()
.filter(tx -> tx.getCreatedTime().before(threeDaysAgo))
.peek(FateTransaction::fail)
.filter(tx -> tx.getCreatedTime().before(fiveDaysAgo))
.forEach(FateTransaction::delete);
```
Returning it as a set and calling `.stream()` on the set would work too. The main point is that `FateTransactionImpl` should have a reference to the client, so you can act on it with `fail()` and `delete()` operations.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
Review Comment:
I think these might be a bit backwards. The transaction ID (what should be returned from a getter of the transaction ID) is a long. The String is merely an prettyPrinted encoding. The method that returns the transaction ID should just be called `getTxid`, and the method that returns the String should be the one that is called something else.
Alternatively, we don't provide any String version, and just add a javadoc that points to Long.toHexString (I think that's what we're using). Or, we could just stop encoding the long in the logs, and just use the number in decimal in the logs.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
Review Comment:
Could just call this `FateTransaction`, since it is a representation of that. I'm not sure adding `Status` to the name adds much value, other than to disambiguate it from internal types that might represent the transaction. The internal types could be renamed, if necessary.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getHeldLocks();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getWaitingLocks();
Review Comment:
Are these lists or sets?
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
Review Comment:
This line makes the current class name very confusing. This object is a status object, with a getStatus method. So, it's returning the status of a status. Also if this is a status code, it could be a type other than String.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getHeldLocks();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getWaitingLocks();
+
+ /**
+ * @return The operation on the top of the stack for this Fate operation.
+ */
+ String getTop();
+
+ /**
+ * @return The timestamp of when the operation was created in ISO format with UTC timezone.
+ */
+ String getTimeCreatedFormatted();
+
+ /**
+ * @return The unformatted form of the timestamp.
+ */
+ long getTimeCreated();
Review Comment:
These should just return a `Date` object or similar. The consumer of this API can format it if they like, using the normal APIs for formatting dates. Only one method is needed to return the date type.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getHeldLocks();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getWaitingLocks();
+
+ /**
+ * @return The operation on the top of the stack for this Fate operation.
+ */
+ String getTop();
Review Comment:
This method could be collapsed with `getStackInfo` into a method that returns an actual stack type (a List or Queue might work in Java), whose top could easily be viewed.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
Review Comment:
This method name is confusing, because it's not clear what a "debug" is as a noun to "get". Expanding it could help: `getDebugInfo` or `getDebuggingInformation`
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1235686797
> Overall, I'm inclined to scrap the idea of adding a FaTE RPC and API so we can access it from the shell. That's the whole basis for these changes... and it's become bloated with all sorts of issues.
I have been tinkering with new FaTE RPCs for an API and I am starting to feel the same way. It doesn't really make sense to have the tablet server execute RPCs when we are just messing with ZK. And we can't have RPCs run in the manager that require it to be down. Also, FaTE is internal to the Manager and was never really meant to be exposed at such a high level.
> I think it should be merged with the more general purpose Admin command.
I like that idea. It looks like FateAdmin has been deprecated for a few versions. Do you think we could drop it for 2.1?
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r902741782
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getHeldLocks();
+
+ /**
+ * @return list of namespace and table ids locked
+ */
+ List<String> getWaitingLocks();
Review Comment:
For some reason the collections for the locks are `List` types. I am not sure why. The method to retrieve them in `AdimUtil` creates Maps with Lists:
```java
Map<Long,List<String>> heldLocks,
Map<Long,List<String>> waitingLocks
```
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r899973993
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
Review Comment:
Yeah, I'm not so sure about using AbstractId either. That class was mainly for things that had a canonical String representation, and very specifically originated as a concrete Table or Namespace ID type... it was never intended as something for all possible identifiers, certainly not to provide a canonical string representation of a numeric 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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r902492553
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
Review Comment:
What about an `AbstractNumericId` or `AbstractLongId`? I feel like there has to be others... Only one I could find was `flushId`:
<pre>
// get the flush id before the new memmap is made available for write
long flushId;
try {
flushId = getFlushID();
</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] EdColeman commented on a diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r929343780
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
Review Comment:
The way that this is used - it is the command name (set when the transaction is seeded in `Fate.seedTransaction` ) something like `getCommand` that clarified that it is the op that created the fate stack.
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r929343780
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
Review Comment:
The way that this is used - it is the command name (set when the transaction is seeded in `Fate.seedTransaction` - something like `getCommand` that clarified that it is the op that created the fate stack.
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r898950536
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
Review Comment:
I agree but I started working on a `FateTxId` class, that extends AbstractId. Checkout my comment on https://github.com/apache/accumulo/issues/2495#issuecomment-1156831152. I like the idea of using a `long` as the ID but I think we really need a class, like we did with `TableId`. We could do a cache, which won't be that useful yet since nothing is using it, but may be eventually. The only thing I wasn't sure about was extending AbstractId. If we store the internal long then we may not need to extend AbstractId. If we do, we have to convert it to a String every time its constructed.
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r898952527
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
+
+ /**
+ * @return The transaction's operation status code.
+ */
+ String getStatus();
Review Comment:
I agree. I am not even sure what this is returning. I am going to add a test, maybe to `FateConcurrencyIT` that uses all of the API endpoints.
--
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 closed pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #2780: New FateOperations API
URL: https://github.com/apache/accumulo/pull/2780
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1235708505
> But we should deprecate the fate stuff in the shell.
What about the new cancel operation?
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r920612966
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransaction.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+import org.apache.accumulo.core.client.AccumuloException;
+import org.apache.accumulo.core.data.FateTxId;
+
+/**
+ * A single transaction of the Fault Tolerant Executor (FaTE) system. A FateTransaction is made up
+ * of multiple operations. When a FateTransaction is created, a unique id is generated and stored in
+ * Zookeeper, represented here has the {@link FateTxId}.
+ */
+public interface FateTransaction {
+ enum Status {
+ /** Unseeded transaction */
+ NEW,
+ /** Transaction that is eligible to be executed */
+ SUBMITTED,
+ /** Transaction that is executing */
+ IN_PROGRESS,
+ /** Transaction has failed, and is in the process of being rolled back */
+ FAILED_IN_PROGRESS,
+ /** Transaction has failed and has been fully rolled back */
+ FAILED,
+ /** Transaction has succeeded */
+ SUCCESSFUL,
+ /** Unrecognized or unknown transaction state */
+ UNKNOWN
+ }
+
+ /**
+ * @return This fate transaction id,
+ */
+ FateTxId getId();
+
+ /**
+ * @return The transaction's operation status code
+ */
+ Status getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
+
Review Comment:
I hope we can come up with a better name / description. I think that this entry is used to (or should) describe the command (target op) that creates the stack. That enables determining the overall command that is executing rather than needing to walk the entire fate stack to find the lowest numbered repo (usually repo_000000) but not sure if that *must always* be the case.
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r904076568
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateOperations.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+import java.util.Set;
+
+import org.apache.accumulo.core.client.AccumuloException;
+
+public interface FateOperations {
+
+ /**
+ * 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(Set<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(Set<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 set of TransactionStatues for corresponding txids
+ * @since 2.1.0
+ */
+ List<FateTransactionStatus> fateStatus(Set<String> txids, List<String> tStatus)
+ throws AccumuloException;
Review Comment:
I think this will work. Here is a method in `FateCommand` that I got to compile:
<pre>
public void failTx(Shell shellState, List<String> txids) throws AccumuloException {
shellState.getAccumuloClient().instanceOperations().fateTransactions().stream()
.filter(fateTransaction -> txids.contains(fateTransaction.getId().canonical().toString()))
.forEach(FateTransaction::fail);
}
</pre>
Some things to note if we go with the `fail()` method hanging off of `FateTransactionImpl`. You will only be able to make calls to fail a single TX at a time. And we won't be able to have any checked exceptions thrown from `fail()`. So the `FateTransaction` interface method will just be:
<pre>
/**
* Fails the fate transaction.
*
* @since 2.1.0
*/
void fail();
</pre>
I guess we could have another instance method that takes a `Set<FateTransaction>` if you want to fail a bunch at once.
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
Manno15 commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1166314121
I am a little behind on following this PR. What else is remaining to get this ready for review?
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1231578372
There are a lot of conflicts in this PR with recent changes. I don't want to loose the design discussion here so I am going to close this in favor of a branch with all the changes merged together.
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1167436667
> I am a little behind on following this PR. What else is remaining to get this ready for review?
It is still a WIP. Working on the new API design.
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
EdColeman commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1239258826
Operators are familiar with the current shell fate operations and may be using them more that expected. I'm not opposed to moving it to the admin command, but the same functionality should be preserved. It would also be nice if the shell could print a message that points to the moved command and at least a hint on how to execute it.
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r904115207
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateOperations.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+import java.util.Set;
+
+import org.apache.accumulo.core.client.AccumuloException;
+
+public interface FateOperations {
+
+ /**
+ * 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(Set<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(Set<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 set of TransactionStatues for corresponding txids
+ * @since 2.1.0
+ */
+ List<FateTransactionStatus> fateStatus(Set<String> txids, List<String> tStatus)
+ throws AccumuloException;
Review Comment:
Failing a bunch is not uncommon - I'm not sure if the shell allows multiples right now, or not, but one way to handle it was to create a command file passed to the shell that had the individual txids / commands:
```
fate fail txid1
fate fail txid2
...
```
It would be friendly if this could be `fate fail txid1 txid2...` in the shell, and if the API supported it.
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r904115207
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateOperations.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+import java.util.Set;
+
+import org.apache.accumulo.core.client.AccumuloException;
+
+public interface FateOperations {
+
+ /**
+ * 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(Set<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(Set<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 set of TransactionStatues for corresponding txids
+ * @since 2.1.0
+ */
+ List<FateTransactionStatus> fateStatus(Set<String> txids, List<String> tStatus)
+ throws AccumuloException;
Review Comment:
Failing a bunch is not uncommon - I'm not sure if the shell allows multiples right now, or not, but one way to handle it was to create a command file passed to the shell that had the individual txids / commands:
```
fate fail txid1
fate fail txid2
...
```
It would be friendly if this could be `fate fail txid1 txid2...` i the shell, and if the API supported it.
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1235669111
The Manager is supposed to be off in order to prevent FaTE concurrency issues during a delicate manual failure-handling situation. If we're directly manipulating FaTE by executing these special methods, we're already in a "special circumstance". Manual manipulation is not a routine operation. I think it makes sense to continue to leave the requirement to have the Manager be down to prevent issues. That's the current requirement, and changing it could introduce a lot of unforeseen concurrency issues.
Overall, I'm inclined to scrap the idea of adding a FaTE RPC and API so we can access it from the shell. That's the whole basis for these changes... and it's become bloated with all sorts of issues. The FateAdmin command functionality never should have been rolled into the shell to begin with. Instead, I think it should be merged with the more general purpose Admin command, like some of the other recent utilities, and removed from the shell. That will satisfy the initial issue we were trying to fix, which was removing the shell's dependency on SiteConfiguration. And, it has the advantage of not needing any of these proposed RPC/API changes, or changing anything about the requirements for online/offline servers. But, it does make it more accessible, alongside the other admin utilities for server-side maintenance.
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1235936071
> > But we should deprecate the fate stuff in the shell.
>
> What about the new cancel operation?
I seem to remember making the point when that was added, that FaTE wasn't really user-facing. It probably makes more sense to expose that as "cancel scheduled compaction" or cancelling some other scheduled operation. Perhaps a cancel flag can be added to the relevant tasks? Alternatively, just put the new cancel option in the Admin command with fail and delete, and roll back the changes made in 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 pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1232020669
> There are a lot of conflicts in this PR with recent changes. I don't want to loose the design discussion here so I am going to close this in favor of a branch with all the changes merged together.
Can you link to the new PR here when it exists, so we can follow the conversation to that 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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
EdColeman commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r920612966
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransaction.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+import org.apache.accumulo.core.client.AccumuloException;
+import org.apache.accumulo.core.data.FateTxId;
+
+/**
+ * A single transaction of the Fault Tolerant Executor (FaTE) system. A FateTransaction is made up
+ * of multiple operations. When a FateTransaction is created, a unique id is generated and stored in
+ * Zookeeper, represented here has the {@link FateTxId}.
+ */
+public interface FateTransaction {
+ enum Status {
+ /** Unseeded transaction */
+ NEW,
+ /** Transaction that is eligible to be executed */
+ SUBMITTED,
+ /** Transaction that is executing */
+ IN_PROGRESS,
+ /** Transaction has failed, and is in the process of being rolled back */
+ FAILED_IN_PROGRESS,
+ /** Transaction has failed and has been fully rolled back */
+ FAILED,
+ /** Transaction has succeeded */
+ SUCCESSFUL,
+ /** Unrecognized or unknown transaction state */
+ UNKNOWN
+ }
+
+ /**
+ * @return This fate transaction id,
+ */
+ FateTxId getId();
+
+ /**
+ * @return The transaction's operation status code
+ */
+ Status getStatus();
+
+ /**
+ * @return The debug info for the operation on the top of the stack for this Fate operation.
+ */
+ String getDebug();
+
Review Comment:
I hope we can come up with a better name / description. I think that this entry is used to (or should) describe the command (target op) that creates the stack. That enables determining the overall command that is executing rather than needing to walk the entire fate stack to find the lowest numbered repo (usually repo_000000) but not sure if that *must always* be the case. When used with `getTop()` the status of the current op and the target command can be determined.
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1167758273
@ctubbsii @EdColeman In my latest changes I added another method to instanceOps and a new enum type to assist with the multiple TXs use case:
```java
void executeFateAction(FateAction action, List<FateTxId> transactions)
throws AccumuloException, AccumuloSecurityException;
```
I started writing the javadoc for the API and realized we need to provide some background about how to use some of the methods, such as `cancel` and `fail`, that can only be used on `FateTransactions` with a certain status. I thought the name `FateAction` is more appropriate for the API, instead of "operation", which refers to the FATE REPO. Then changed the Thrift types to reflect this naming. I also created an enum in FateTransaction for `Status` that directly mirrors the internal enum `TStatus` in `ReadOnlyTStore`. Let me know what you think.
--
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 #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1235693602
> I like that idea. It looks like FateAdmin has been deprecated for a few versions. Do you think we could drop it for 2.1?
No. We can drop it in 3, along with the fate stuff in the shell. But we should deprecate the fate stuff in 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] milleruntime commented on pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#issuecomment-1235471469
@ctubbsii what do you think the special methods, `fail()` and `delete()` being called on a `FateTransaction`? These currently require the Manager to be down to free the lock, which I feel is a special circumstance. A user can easily get the `Set<FateTransaction>` while the Manager is running and call these methods. We could easily throw an error if the Manager is running but it seems like that could trip up users.
--
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 diff in pull request #2780: New FateOperations API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2780:
URL: https://github.com/apache/accumulo/pull/2780#discussion_r929339868
##########
core/src/main/java/org/apache/accumulo/core/client/admin/FateTransactionStatus.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.List;
+
+/**
+ * FATE transaction status, including lock information.
+ */
+public interface FateTransactionStatus {
+
+ /**
+ * @return This fate operations transaction id, formatted in the same way as FATE transactions are
+ * in the Accumulo logs.
+ */
+ String getTxid();
+
+ /**
+ * @return This fate operations transaction id, in its original long form.
+ */
+ long getTxidLong();
Review Comment:
An abstract numeric ID would certainly be better than trying to reuse the string-based one... but I don't think the need for such a thing is particularly compelling 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