You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/12/21 16:49:06 UTC
[GitHub] [accumulo] foster33 opened a new pull request #2397: Add consistency checks for lastFlushID and lastCompactID
foster33 opened a new pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397
Fixes #2153
This PR addresses the following:
- Creates a consistency check for `getFlushID` & `lastFlushID`
- Creates a consistency check for `getCompactionID` & `lastCompacyID`
- Removes TODO relating to change
- Fixes a minor misspelling in the area of the original change
In my changes I have created two try / catch blocks surrounding the getFlushID and getCompactionID assignments. One for each consistency check. I was not sure whether the NoNodeException should be thrown, logged or just printed. If anyone has any input regarding that, I would appreciate 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] keith-turner commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r779172680
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
+ try {
+ long flushID = getFlushID();
Review comment:
> Would this be a similar process for COMPACT_ID?
Yeah I think 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
[GitHub] [accumulo] foster33 commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r778956966
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
+ try {
+ long flushID = getFlushID();
Review comment:
Thank you, that is very helpful!
Would this be a similar process for COMPACT_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] dlmarion commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r773875802
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
Review comment:
I'm not sure that this comment is correct. In fact, I'm not sure that the original comment from 2013 applies at this point. From what my IDE search found, ZTABLE_FLUSH_ID is only set in [ManagerClientServiceHandler#initiateFlush](https://github.com/apache/accumulo/blob/main/server/manager/src/main/java/org/apache/accumulo/manager/ManagerClientServiceHandler.java#L125) which IIRC is used when someone submits a `flush` command in the shell. From what I can tell, if a Tablet is hosted on a TabletServer in the normal course of business and minor compactions occur naturally, then `lastFlushID` will be equal to the value passed in the Tablet constructor and `flushID` will be equal to the value stored in ZooKeeper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r800194843
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
Review comment:
From what I recall when I moved the issue into GitHub, I did notice certain circumstances which led to lastFlushID and getFlushID showing that no flush has occurred but still not being consistent with each other. I will re-investigate to see if that still holds true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r788027850
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
Review comment:
@dlmarion Is this, and your other comment below resolved with the updated changes? Is this PR ready to be merged in your opinion?
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#issuecomment-1046326584
This change broke `org.apache.accumulo.test.functional.DeleteRowsIT.testManyRows`, and I'm not sure why. This may need to be reverted until it is solved.
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r777733833
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
+ try {
+ long flushID = getFlushID();
Review comment:
this will read the current table wide flush ID from ZK. That is not what we want to check here. Instead, we want to check if lastFlushId matches what is stored in the metdata table for this tablet. The lastFlushId instance var should contain whats in the metadata table and it should be updated in memory when the metadata table is updated. We can change the Ample code that reads the tablet metadata to include flush id and then do something like the following.
```java
if(tabletMeta.getFlushId().orElse(-1) != lastFlushId) {
// mismatch with tablet metadata , so throw exception
}
```
Not sure if the case where flushId was never set it correct in the code above. Need to investigate further.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r773324035
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,32 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ try {
+ long flushID = getFlushID();
+ if (lastFlushID != 0 && flushID == 0) {
+ String msg = "Closed tablet " + extent + " has not been flushed before being closed.";
+ log.error(msg);
+ throw new RuntimeException(msg);
+ }
+ } catch (NoNodeException e) {
+ e.printStackTrace();
Review comment:
Didn't look at the rest of the PR, but one quick note: we'd want to log or throw these exceptions, rather than print them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r783204419
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1402,6 +1402,20 @@ private void closeConsistencyCheck() {
throw new RuntimeException(msg);
}
+ if (tabletMeta.getFlushId().orElse(-1) != lastFlushID) {
Review comment:
Last week I examined to lastFlushID in the tablet server code to see if it is -1 when not set. Based on my analysis, it is -1 so doing orElse(-1) is ok here. I don't think I analyzed lastCompactID. If it has not been done, it would be good to analyze initialization of both of them as my analysis was quick.
This -1 practice is extremely brittle. A good follow-on PR would be to do something else (constant vs -1, OptionalLong, Long+null, ??) to make it less brittle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r800197138
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
Review comment:
From just a quick test run with accumulo-testing ingesting and things flushing normally (no compactions happened yet), I saw lastFlushID 0, last CompactID -1, getCompactID optionalLong empty, and getFlushID opt long 0. The states match up but I am not sure the consistency checks here will pass through consistently.
--
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 merged pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
Manno15 merged pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r788027198
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1402,6 +1402,20 @@ private void closeConsistencyCheck() {
throw new RuntimeException(msg);
}
+ if (tabletMeta.getFlushId().orElse(-1) != lastFlushID) {
Review comment:
Is there more to be done here? I see @keith-turner approved this. Is this ready to merge?
--
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] foster33 commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r783899687
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1402,6 +1402,20 @@ private void closeConsistencyCheck() {
throw new RuntimeException(msg);
}
+ if (tabletMeta.getFlushId().orElse(-1) != lastFlushID) {
Review comment:
I will go ahead and analyze the initialization of both lastFlushID and lastCompactID.
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r783204419
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1402,6 +1402,20 @@ private void closeConsistencyCheck() {
throw new RuntimeException(msg);
}
+ if (tabletMeta.getFlushId().orElse(-1) != lastFlushID) {
Review comment:
Last week I examined to lastFlushID in the tablet server code to see if it is -1 when not set. Based on my analysis, it is -1 so doing orElse(-1) is ok here. I don't think I analyzed lastCompactID. If it has not been done, it would be good to analyze initialization of both of them as my analysis was quick.
This -1 practice is extremely brittle. A good follow on PR would be to do something else (constants, OptionalLong, ??) in a follow on PR to make it less brittle.
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r783204419
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1402,6 +1402,20 @@ private void closeConsistencyCheck() {
throw new RuntimeException(msg);
}
+ if (tabletMeta.getFlushId().orElse(-1) != lastFlushID) {
Review comment:
Last week I examined to lastFlushID in the tablet server code to see if it is -1 when not set. Based on my analysis, it is -1 so doing orElse(-1) is ok here. I don't think I analyzed lastCompactID. If it has not been done, it would be good to analyze initialization of both of them as my analysis of the one was quick.
This -1 practice is extremely brittle. A good follow-on PR would be to do something else (constant vs -1, OptionalLong, Long+null, ??) to make it less brittle.
--
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] dlmarion commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r773880985
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
+ try {
+ long flushID = getFlushID();
+ if (lastFlushID != 0 && flushID == 0) {
+ String msg = "Closed tablet " + extent + " has not been flushed before being closed.";
+ log.error(msg);
+ throw new RuntimeException(msg);
+ }
+ } catch (NoNodeException e) {
+ String msg = "Failed to do close consistency check for tablet " + extent;
+ log.error(msg, e);
+ throw new RuntimeException(msg, e);
+ }
+
+ // If a table hasn't been compacted before it was closed then lastCompactID will be -1 while
+ // getCompactionID will be 0.
Review comment:
Same thing here. `Z_COMPACT_ID` is set to `0` when the table is created and incremented when a user manually compacts that table.
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#issuecomment-1047006634
Seems the issue is the if statement is true so it throws the RuntimeException. Which doesn't exit the test but rather slows things down due to the retries. This eventually causes the test to timeout. I can get a fix out next weekend though if someone else wants to tackle it in the meantime, feel free.
```
2022-02-21T10:28:17,562 [tablet.Tablet] ERROR: Closed tablet 1;n< lastFlushID is inconsistent with metadata : -1 != 0
2022-02-21T10:28:17,562 [tablet.Tablet] ERROR: Failed to do close consistency check for tablet 1;n<
java.lang.RuntimeException: Closed tablet 1;n< lastFlushID is inconsistent with metadata : -1 != 0
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#issuecomment-1046342542
I will look into it when I get the chance. Feel free to revert if I don't get to it in time.
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r778937443
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
+ try {
+ long flushID = getFlushID();
Review comment:
> I was curious what part of code that may be.
There is some code like the following earlier in the method that uses Ample to read the tablets metadata. Can add the FLUSH_ID to that code.
```java
var tabletMeta = context.getAmple().readTablet(extent, ColumnType.FILES, ColumnType.LOGS,
ColumnType.ECOMP, ColumnType.PREV_ROW, ColumnType.FLUSH_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] dlmarion commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r773883326
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
Review comment:
I would like someone else to confirm what I'm seeing. There may be a different way to do the close consistency check for flushes and compactions, I just think the information in the original issue, which this change is based off of, is out of date.
--
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] foster33 commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r773880797
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
Review comment:
That could very well be true. I based the comments off of the information that was provided in the original issue on GitHub. I can remove this if it is inaccurate or outdated or change the wording.
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r779964415
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1402,6 +1402,18 @@ private void closeConsistencyCheck() {
throw new RuntimeException(msg);
}
+ if (tabletMeta.getFlushId().orElse(-1) != lastFlushID) {
+ String msg = "Closed tablet " + extent + " has not been flushed before being closed.";
+ log.error(msg);
+ throw new RuntimeException(msg);
+ }
+
+ if (tabletMeta.getCompactId().orElse(-1) != lastCompactID) {
+ String msg = "Closed tablet " + extent + " has not been compacted before being closed.";
Review comment:
```suggestion
String msg = "Closed tablet " + extent + " lastCompactID is inconsistent with metdata "+tabletMeta.getCompactId().orElse(-1)+ "!="+ lastCompactID
```
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1402,6 +1402,18 @@ private void closeConsistencyCheck() {
throw new RuntimeException(msg);
}
+ if (tabletMeta.getFlushId().orElse(-1) != lastFlushID) {
+ String msg = "Closed tablet " + extent + " has not been flushed before being closed.";
Review comment:
The flush status is uncertain here, all we know is that this in memory data is inconsistent with the metadata table.
```suggestion
String msg = "Closed tablet " + extent + " lastFlushId is inconsistent with metadata : "+(tabletMeta.getFlushId().orElse(-1)+" !=" +lastFlushID;
```
--
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] foster33 commented on a change in pull request #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
foster33 commented on a change in pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#discussion_r778889049
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
##########
@@ -1412,12 +1412,40 @@ private void closeConsistencyCheck() {
if (!otherLogs.isEmpty() || !currentLogs.isEmpty() || !referencedLogs.isEmpty()) {
String msg = "Closed tablet " + extent + " has walog entries in memory currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = " + referencedLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " referencedLogs = " + referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
- // TODO check lastFlushID and lostCompactID - ACCUMULO-1290
+ // If a table hasn't been flushed before it was closed then lastFlushID will be -1 while
+ // getFlushID will be 0.
+ try {
+ long flushID = getFlushID();
Review comment:
So I was looking into this and I have some questions.
You mentioned that we can change the Ample code that reads the tablet metadata to include flushID and do what you had suggested. I was curious what part of code that may be. I could not identify it, but I did see that Ample already had knowledge of flushID in general.
Would the change just deal with how the variable is updated? Or would it be dealing with something more complex?
--
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 #2397: Add consistency checks for lastFlushID and lastCompactID
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2397:
URL: https://github.com/apache/accumulo/pull/2397#issuecomment-1046342989
Okay, I'll revert it then, because I don't want it to hold up other changes, but it should be simply to re-apply once the problem is discovered. I'll re-open the original issue to track 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