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/02/09 15:34:53 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2475: Refactor fate
milleruntime opened a new pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475
Some ideas on how we can clean up FATE and implement #1249
--
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] keith-turner commented on a change in pull request #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#discussion_r804950538
##########
File path: core/src/main/java/org/apache/accumulo/fate/Repo.java
##########
@@ -34,16 +34,11 @@ default String getReturn() {
return null;
}
- // return a string of the current status
- default String getStatus() {
- return "WARNING: Status has not been implemented yet.";
- }
-
// return the Goal of the current operation
default Goal getGoal() {
- return new Goal("WARNING: Status has not been implemented yet.");
+ return new Goal(0, 0, "WARNING: Operation has not implemented a goal.");
Review comment:
I think it would be a useful feature to have in 2.1 if we can get it done. Thinking about it a bit more I am puzzling over how the status info would stored in ZK. Was thinking it would go w/ the stack of repos, however in the case of undoing repos the status info would be lost. Under normal circumstances repos are pushed onto a per fate tx stack in ZK as they are executed, so this stack could also have status info. However when there is an error fate will start popping up repos off the stack and executing undo ops on them. Because of this thinking it may make more sense to just have a per fate transaction list of status strings/objects in ZK.
--
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] keith-turner commented on pull request #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#issuecomment-1084844059
I plan to circle back to this next week and review it again.
--
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 #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#discussion_r804092483
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java
##########
@@ -120,28 +118,26 @@ public long isReady(long tid, Manager manager) throws Exception {
long scanTime = System.currentTimeMillis() - t1;
- Tables.clearCache(manager.getContext());
- if (tabletCount == 0 && !Tables.exists(manager.getContext(), tableId))
+ Tables.clearCache(env.getContext());
+ if (tabletCount == 0 && !Tables.exists(env.getContext(), tableId))
throw new AcceptableThriftTableOperationException(tableId.canonical(), null,
TableOperation.COMPACT, TableOperationExceptionType.NOTFOUND, null);
if (serversToFlush.size() == 0
- && Tables.getTableState(manager.getContext(), tableId) == TableState.OFFLINE)
+ && Tables.getTableState(env.getContext(), tableId) == TableState.OFFLINE)
throw new AcceptableThriftTableOperationException(tableId.canonical(), null,
TableOperation.COMPACT, TableOperationExceptionType.OFFLINE, null);
if (tabletsToWaitFor == 0)
return 0;
- for (TServerInstance tsi : serversToFlush.keySet()) {
- try {
- final TServerConnection server = manager.getConnection(tsi);
- if (server != null)
- server.compact(manager.getManagerLock(), tableId.canonical(), startRow, endRow);
- } catch (TException ex) {
- LoggerFactory.getLogger(CompactionDriver.class).error(ex.toString());
- }
- }
+ // TODO figure this out
Review comment:
@ctubbsii here is another place where it could be nice to have `context.getZooReaderWriter()` in ClientContext. If I move `LiveTServerSet` to core, it uses `ServerContext` and writes to ZK.
--
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 #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#discussion_r804582728
##########
File path: core/src/main/java/org/apache/accumulo/fate/Repo.java
##########
@@ -34,16 +34,11 @@ default String getReturn() {
return null;
}
- // return a string of the current status
- default String getStatus() {
- return "WARNING: Status has not been implemented yet.";
- }
-
// return the Goal of the current operation
default Goal getGoal() {
- return new Goal("WARNING: Status has not been implemented yet.");
+ return new Goal(0, 0, "WARNING: Operation has not implemented a goal.");
Review comment:
Thanks for the ideas. I hadn't thought of that.
--
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 #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#issuecomment-1062239639
> > > @keith-turner I ran into a dependency snag. The FateCommand uses a bunch of Fate types and is in the Shell package. Moving the Fate stuff to manager, breaks this. I don't know of a good solution other than rewrite FateCommand and make new types that use API. Any ideas?
> >
> >
> > @milleruntime I missed this comment. I have not looked at this in a while, I can take a fresh look at it. Is this PR in a state where you want a review or are you still changing some things?
>
> Yeah it is in a good state, if you want to take a look at it. This PR isn't a priority though as it will probably have to go into 3.0. I pulled some of these changes out into #2541. Also, @Manno15 is working on changes here: #2215, which should be able to get into 2.1 but have some new API additions you might want to look at as well.
If you want to wait for me to merge the latest changes in main, I am working on that now.
--
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 #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#discussion_r804090090
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactionDriver.java
##########
@@ -120,28 +118,26 @@ public long isReady(long tid, Manager manager) throws Exception {
long scanTime = System.currentTimeMillis() - t1;
- Tables.clearCache(manager.getContext());
- if (tabletCount == 0 && !Tables.exists(manager.getContext(), tableId))
+ Tables.clearCache(env.getContext());
+ if (tabletCount == 0 && !Tables.exists(env.getContext(), tableId))
throw new AcceptableThriftTableOperationException(tableId.canonical(), null,
TableOperation.COMPACT, TableOperationExceptionType.NOTFOUND, null);
if (serversToFlush.size() == 0
- && Tables.getTableState(manager.getContext(), tableId) == TableState.OFFLINE)
+ && Tables.getTableState(env.getContext(), tableId) == TableState.OFFLINE)
throw new AcceptableThriftTableOperationException(tableId.canonical(), null,
TableOperation.COMPACT, TableOperationExceptionType.OFFLINE, null);
if (tabletsToWaitFor == 0)
return 0;
- for (TServerInstance tsi : serversToFlush.keySet()) {
- try {
- final TServerConnection server = manager.getConnection(tsi);
- if (server != null)
- server.compact(manager.getManagerLock(), tableId.canonical(), startRow, endRow);
- } catch (TException ex) {
- LoggerFactory.getLogger(CompactionDriver.class).error(ex.toString());
- }
- }
+ // TODO figure this out
Review comment:
This is a problem. The `LiveTServerSet` is maintained in the Manager. I might be able to move the class to core.
--
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] keith-turner commented on pull request #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#issuecomment-1061224067
> @keith-turner I ran into a dependency snag. The FateCommand uses a bunch of Fate types and is in the Shell package. Moving the Fate stuff to manager, breaks this. I don't know of a good solution other than rewrite FateCommand and make new types that use API. Any ideas?
@milleruntime I missed this comment. I have not looked at this in a while, I can take a fresh look at it. Is this PR in a state where you want a review or are you still changing some things?
--
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 #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#issuecomment-1061714928
> > @keith-turner I ran into a dependency snag. The FateCommand uses a bunch of Fate types and is in the Shell package. Moving the Fate stuff to manager, breaks this. I don't know of a good solution other than rewrite FateCommand and make new types that use API. Any ideas?
>
> @milleruntime I missed this comment. I have not looked at this in a while, I can take a fresh look at it. Is this PR in a state where you want a review or are you still changing some things?
Yeah it is in a good state, if you want to take a look at it. This PR isn't a priority though as it will probably have to go into 3.0. I pulled some of these changes out into https://github.com/apache/accumulo/pull/2541. Also, @Manno15 is working on changes here: https://github.com/apache/accumulo/pull/2215, which should be able to get into 2.1 but have some new API additions you might want to look at 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] keith-turner commented on a change in pull request #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#discussion_r804198315
##########
File path: core/src/main/java/org/apache/accumulo/fate/Repo.java
##########
@@ -34,16 +34,11 @@ default String getReturn() {
return null;
}
- // return a string of the current status
- default String getStatus() {
- return "WARNING: Status has not been implemented yet.";
- }
-
// return the Goal of the current operation
default Goal getGoal() {
- return new Goal("WARNING: Status has not been implemented yet.");
+ return new Goal(0, 0, "WARNING: Operation has not implemented a goal.");
Review comment:
In #1249 I was thinking the goal information could be provided when a fate transaction is seeded and stored in ZK. It would not be obtained from each repo, instead status information could be related with each repo in the stack in ZK. So to get the information for a fate transaction, get the goal for the fate transaction and then travese the stack getting the status information for each repo.
--
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] keith-turner commented on a change in pull request #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#discussion_r804201020
##########
File path: core/src/main/java/org/apache/accumulo/fate/Repo.java
##########
@@ -34,16 +34,11 @@ default String getReturn() {
return null;
}
- // return a string of the current status
- default String getStatus() {
- return "WARNING: Status has not been implemented yet.";
- }
-
// return the Goal of the current operation
default Goal getGoal() {
- return new Goal("WARNING: Status has not been implemented yet.");
+ return new Goal(0, 0, "WARNING: Operation has not implemented a goal.");
Review comment:
For example maybe [this method](https://github.com/apache/accumulo/blob/7088cdec4fab5bb590645ab6dc10fb7b2021f50e/core/src/main/java/org/apache/accumulo/fate/Fate.java#L271) could be changed to :
```java
public void seedTransaction(long tid, Repo<T> repo, boolean autoCleanUp, String goalMessage)
```
--
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 #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#discussion_r804870549
##########
File path: core/src/main/java/org/apache/accumulo/fate/Repo.java
##########
@@ -34,16 +34,11 @@ default String getReturn() {
return null;
}
- // return a string of the current status
- default String getStatus() {
- return "WARNING: Status has not been implemented yet.";
- }
-
// return the Goal of the current operation
default Goal getGoal() {
- return new Goal("WARNING: Status has not been implemented yet.");
+ return new Goal(0, 0, "WARNING: Operation has not implemented a goal.");
Review comment:
Also what do you think of trying to get the status reporting into 2.1 in a separate change? Seems like adding information to the serialized nodes shouldn't break whats already 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] milleruntime commented on pull request #2475: Refactor fate
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2475:
URL: https://github.com/apache/accumulo/pull/2475#issuecomment-1044830647
@keith-turner I ran into a dependency snag. The FateCommand uses a bunch of Fate types and is in the Shell package. Moving the Fate stuff to manager, breaks this. I don't know of a good solution other than rewrite `FateCommand` and make new types that use API. Any ideas?
--
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