You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/09/26 15:37:44 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3779: New data version to support no-chop changes, disable upgrade
EdColeman opened a new pull request, #3779:
URL: https://github.com/apache/accumulo/pull/3779
Adds a new Accumulo data version (12) to reflect no-chop changes that encode the metadata file entries in json to support fencing.
The json file entries are incompatible with metadata file entries prior to 3.1. Until the upgrade code is implemented trying to upgrade from a previous version will fail. This change makes it explicit why.
For example, with this change, trying to upgrade from a 2.1.x will include the following in the manager log.
```
2023-09-26T14:34:03,211 [start.Main] ERROR: Thread 'manager' died.
java.lang.IllegalStateException: This version of accumulo (3.1.0-SNAPSHOT) is not compatible with files stored using data version 10. Please upgrade from 3.1.0 or later.
at org.apache.accumulo.server.ServerContext.ensureDataVersionCompatible(ServerContext.java:308) ~[accumulo-server-base-3.1.0-SNAPSHOT.jar:3.1.0-SNAPSHOT]
```
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1337754453
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
When you say you opted to make this test fail instead, do you mean when the fix for #3768 is eventually done? It looks like you have it set to pass right now, but should fail after the upgrade is done on line 146 with:
```java
IntStream.of(oldestSupported - 1, currentVersion + 1).forEach(shouldFail);
```
Is that what you mean?
If so, I think that's fine. Otherwise, I'm confused.
--
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 merged pull request #3779: New data version to support no-chop changes, disable upgrade
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman merged PR #3779:
URL: https://github.com/apache/accumulo/pull/3779
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1337806933
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
Yes. When adding the list of supported versions in `UpgradeCoordinator` the forEach(should fail) kicks out (line my off by one with my version)
```
org.opentest4j.AssertionFailedError: Expected java.lang.IllegalStateException to be thrown, but nothing was thrown.
at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:73)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:35)
at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3083)
at org.apache.accumulo.server.ServerContextTest.lambda$testCanRun$3(ServerContextTest.java:144)
at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1032)
at java.base/java.util.stream.IntPipeline$Head.forEach(IntPipeline.java:593)
at org.apache.accumulo.server.ServerContextTest.testCanRun(ServerContextTest.java:147)
...
```
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1339075662
##########
server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java:
##########
@@ -71,7 +77,10 @@ public static int get() {
return CURRENT_VERSION;
}
- public static final Set<Integer> CAN_RUN = Set.of(ROOT_TABLET_META_CHANGES, CURRENT_VERSION);
+ // TODO - this disables upgrades until https://github.com/apache/accumulo/issues/3768 is done
+ // public static final Set<Integer> CAN_RUN = Set.of(REMOVE_DEPRECATIONS_FOR_VERSION_3,
Review Comment:
I will make the change. As luck would have it I already have the correct code in another branch ;-)
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#issuecomment-1736215709
`Curious if the manager stops there` It looks like the manager, tservers and gc hit the data version check and immediately bail with the ERROR / IllegalStateException provided above.
Basically, as soon as a service can read from HDFS it is checking the data version and then throwing the exception, halting the service before any changes / operation can be performed.
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1337809055
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
Yes - when adding the supporting versions to `UpgradeCoordinator` the line
```
IntStream.of(oldestSupported - 1, currentVersion + 1).forEach(shouldFail);
```
causes a hard fail. (My line numbers may be off by one.)
```
org.opentest4j.AssertionFailedError: Expected java.lang.IllegalStateException to be thrown, but nothing was thrown.
at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:73)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:35)
at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3083)
at org.apache.accumulo.server.ServerContextTest.lambda$testCanRun$3(ServerContextTest.java:144)
at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1032)
at java.base/java.util.stream.IntPipeline$Head.forEach(IntPipeline.java:593)
at org.apache.accumulo.server.ServerContextTest.testCanRun(ServerContextTest.java:147)
```
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#issuecomment-1736039237
> For example, with this change, trying to upgrade from a 2.1.x will include the following in the manager log.
That is kinda neat. Curious if the manager stops there, or does it still continue to do things like start fate and start the thrift client service.
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1337806933
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
Yes. When adding the list of supported versions in `UpgradeCoordinator` the forEach(should fail) kicks out (line my off by one with my version)
```
org.opentest4j.AssertionFailedError: Expected java.lang.IllegalStateException to be thrown, but nothing was thrown.
at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:73)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:35)
at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3083)
at org.apache.accumulo.server.ServerContextTest.lambda$testCanRun$3(ServerContextTest.java:144)
at java.base/java.util.Spliterators$IntArraySpliterator.forEachRemaining(Spliterators.java:1032)
at java.base/java.util.stream.IntPipeline$Head.forEach(IntPipeline.java:593)
at org.apache.accumulo.server.ServerContextTest.testCanRun(ServerContextTest.java:147)
...
```
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1337754453
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
When you say you opted to make this test fail instead, do you mean when the fix for #3768 is eventually upgraded? It looks like you have it set to pass right now, but should fail after the upgrade is done on line 146 with:
```java
IntStream.of(oldestSupported - 1, currentVersion + 1).forEach(shouldFail);
```
Is that what you mean?
If so, I think that's fine. Otherwise, I'm confused.
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#issuecomment-1736147396
> > For example, with this change, trying to upgrade from a 2.1.x will include the following in the manager log.
>
> That is kinda neat. Curious if the manager stops there, or does it still continue to do things like start fate and start the thrift client service.
I'm pretty sure it won't start fate until the upgrade is complete, but I'm not sure about the client service.
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1337676816
##########
server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java:
##########
@@ -71,7 +77,11 @@ public static int get() {
return CURRENT_VERSION;
}
- public static final Set<Integer> CAN_RUN = Set.of(ROOT_TABLET_META_CHANGES, CURRENT_VERSION);
+ // TODO - this is currently set to disable upgrades until metadata file json conversion
+ // implemented
+ // public static final Set<Integer> CAN_RUN = Set.of(REMOVE_DEPRECATIONS_FOR_VERSION_3,
+ // CURRENT_VERSION);
Review Comment:
```suggestion
// TODO - this disables upgrades until https://github.com/apache/accumulo/issues/3768 is done
// public static final Set<Integer> CAN_RUN = Set.of(REMOVE_DEPRECATIONS_FOR_VERSION_3,
// CURRENT_VERSION);
```
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
Instead of doing this, you should use `@Disabled`
```suggestion
final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
```
On the method, add:
```java
@Disabled("must fix https://github.com/apache/accumulo/issues/3768 to re-enable")
```
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1339051771
##########
server/base/src/main/java/org/apache/accumulo/server/AccumuloDataVersion.java:
##########
@@ -71,7 +77,10 @@ public static int get() {
return CURRENT_VERSION;
}
- public static final Set<Integer> CAN_RUN = Set.of(ROOT_TABLET_META_CHANGES, CURRENT_VERSION);
+ // TODO - this disables upgrades until https://github.com/apache/accumulo/issues/3768 is done
+ // public static final Set<Integer> CAN_RUN = Set.of(REMOVE_DEPRECATIONS_FOR_VERSION_3,
Review Comment:
If I correctly remember how this works, this should include the data version from 2.1 and 3.0, when it is uncommented later.
```suggestion
// public static final Set<Integer> CAN_RUN = Set.of(ROOT_TABLET_META_CHANGES, REMOVE_DEPRECATIONS_FOR_VERSION_3,
```
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
Okay, in that case, I think it would be more clear that it is disabled by referencing the current version instead of the name of the version. Also, can add the commented out version that it needs to go back to when it is fixed later.
```suggestion
// TODO basically disable check until upgrade to 3.1 is supported.
// final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
final int oldestSupported = AccumuloDataVersion.CURRENT_VERSION;
```
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1337716868
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
I thought of using disabled - opted to make this test fail instead. This way, as the code is changed to support the necessary version, this test will fail and be impossible to overlook.
--
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 #3779: New data version to support no-chop changes, disable upgrade
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3779:
URL: https://github.com/apache/accumulo/pull/3779#discussion_r1339598756
##########
server/base/src/test/java/org/apache/accumulo/server/ServerContextTest.java:
##########
@@ -135,7 +135,9 @@ public void testCanRun() {
// ensure this fails with older versions; the oldest supported version is hard-coded here
// to ensure we don't unintentionally break upgrade support; changing this should be a conscious
// decision and this check will ensure we don't overlook it
- final int oldestSupported = AccumuloDataVersion.ROOT_TABLET_META_CHANGES;
+
+ // TODO basically disable check until upgrade to 3.1 is supported.
+ final int oldestSupported = AccumuloDataVersion.METADATA_FILE_JSON_ENCODING;
Review Comment:
I don't think you included my suggestion to make the test point directly to the `CURRENT_VERSION` alias. You left it pointing to `METADATA_FILE_JSON_ENCODING`. I know they are the same value, but I think that leaving it pointing to the specific named version implies there's some reason that specific version was picked. But the reason it was picked is because it's the current version. So, it's more clear to just directly point to `CURRENT_VERSION`. But it doesn't matter that much, because this should all get backed out once the upgrade code is added, so I don't think it's worth changing now. I just wanted to explain my reasoning a bit more.
--
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