You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2025/06/13 19:19:17 UTC
[PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
dlmarion opened a new pull request, #5646:
URL: https://github.com/apache/accumulo/pull/5646
Re-check that the tablet metadata has a migration and return false if not instead of throwing an exception.
Closes #5643
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2145994785
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
Yeah, this is strange. It doesn't seem possible that migration could be null here...
Filter seems good and the case you mentioned seems unlikely... That sounds like that would be a bug with `TabletsMetadata`
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2145935396
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
I'm not sure if this is a permanent fix.
We could merge this in as a temporary fix though.
Should verify the filter is doing what we think it's doing. If it's the case that "the migration was removed after the filter ran, but before the TabletMetadata was returned, by some other process." like you said, this might be an okay permanent fix.
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2145943958
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
Regarding, "the migration was removed after the filter ran, but before the TabletMetadata was returned, by some other process," I'm not sure if that's the case or not. It's a guess because the filter is pretty simple. We likely need @keith-turner to weigh in on this one.
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2145926677
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
I see the issue you linked... Looking 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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion closed pull request #5646: Returned false from Manager.shouldCleanupMigration vs exception
URL: https://github.com/apache/accumulo/pull/5646
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2149716762
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
I think I figured out the issue. I don't believe that any of the filters are being run on the root tablet metadata. I'm likely going to put up a different PR and close this one, but I'll leave this open for 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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2145924579
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
Were you seeing an IllegalStateException? Something is wrong in the current impl if so
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2145925734
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
See the associated issue for stack traces
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#issuecomment-2976574050
Closing in favor of #5648
--
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
Re: [PR] Returned false from Manager.shouldCleanupMigration vs exception [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #5646:
URL: https://github.com/apache/accumulo/pull/5646#discussion_r2146006011
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -582,10 +582,11 @@ public void run() {
}
private boolean shouldCleanupMigration(TabletMetadata tabletMetadata) {
+ if (!tabletMetadata.hasMigration()) {
+ return false;
+ }
var tableState = getContext().getTableManager().getTableState(tabletMetadata.getTableId());
var migration = tabletMetadata.getMigration();
- Preconditions.checkState(migration != null,
- "This method should only be called if there is a migration");
Review Comment:
So, the filter runs on the metadata tablet server and then the tablet metadata is shipped back to the caller, the Manager in this case. I'm not sure if a modification to the tablet metadata in between the evaluation and the transfer can occur or not.
--
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