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/01/06 10:06:25 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request #2410: Fix MiniAccumuloClusterImpl.verifyUp()
ctubbsii opened a new pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410
Fix verifyUp method broken by #2408. The previous code attempted to use
a client singleton resource (ZooSession, created inside ZooReaderWriter)
without instantiating an AccumuloClient. This fix ensures that resource
is created after MiniAccumuloClusterImpl's ServerContext has been
created, so a client context is established before the client singleton
resource is used.
--
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 #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410#issuecomment-1009309410
> @ctubbsii - I removed the use of ZooReaderWriter from MiniAccumuloClusterImpl.verifyUp. Instead, it uses the Native ZK client. Change [here](https://github.com/dlmarion/accumulo/commit/d514b7be3e6924ed23a618e36657989f9eaf340b). Running AccumuloClientIT on the command line yeilds:
>
> ```
> java.lang.AssertionError: expected:<CLIENT> but was:<SERVER>
> at org.junit.Assert.fail(Assert.java:89)
> at org.junit.Assert.failNotEquals(Assert.java:835)
> at org.junit.Assert.assertEquals(Assert.java:120)
> at org.junit.Assert.assertEquals(Assert.java:146)
> at org.apache.accumulo.test.functional.AccumuloClientIT.testClose(AccumuloClientIT.java:191)
> ```
>
> I'm not sure what's going on here.
The call to `getServerContext` is the problematic part. You avoided it by removing my `getServerContext().getZooReaderWriter()` and `getServerContext().getZooKeeperRoot()`, but then you added it back in for `getServerContext().getConfiguration()` and `getServerContext().getInstanceID()`.
If the solution is to use ZooKeeper's API directly instead of our own, then we have to do it using the config for MiniAccumuloClusterImpl without instantiating the ServerContext because that's the part that sets the singleton mode to SERVER.
--
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 #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410#issuecomment-1016735091
For reference, the fix is being worked in #2414
--
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 pull request #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410#issuecomment-1009273719
@ctubbsii - I removed the use of ZooReaderWriter from MiniAccumuloClusterImpl.verifyUp. Instead, it uses the Native ZK client. Change [here](https://github.com/dlmarion/accumulo/commit/d514b7be3e6924ed23a618e36657989f9eaf340b). Running AccumuloClientIT on the command line yeilds:
```
java.lang.AssertionError: expected:<CLIENT> but was:<SERVER>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at org.apache.accumulo.test.functional.AccumuloClientIT.testClose(AccumuloClientIT.java:191)
```
I'm not sure what's going on here.
--
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 #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410#issuecomment-1007659778
@dlmarion Apparently, this fixed it for most of the tests, but there's at least one test that this breaks that I'll have to figure out: AccumuloClientIT.testClose asserts that we're in a CLIENT mode, but MiniAccumuloCluster specifically uses SERVER mode. The test passed before because mini lazily created a ServerContext that put it in SERVER mode. With these changes, the ServerContext is created sooner than it would have been.
There's a few other tests that failed as a result of my fix as well (not anywhere near the 139 that were failing before, though), but those could just be flakes. I'd have to investigate. Including the one I already mentioned, the list is:
```
org.apache.accumulo.test.functional.AccumuloClientIT.testClose
org.apache.accumulo.test.functional.CleanUpIT.run
org.apache.accumulo.test.functional.DurabilityIT.testIncreaseDurability
org.apache.accumulo.test.functional.LateLastContactIT.test
org.apache.accumulo.test.functional.ReadWriteIT.sunnyDay
org.apache.accumulo.test.functional.RestartIT.restartManagerRecovery
```
--
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 edited a comment on pull request #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment on pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410#issuecomment-1009309410
> @ctubbsii - I removed the use of ZooReaderWriter from MiniAccumuloClusterImpl.verifyUp. Instead, it uses the Native ZK client. Change [here](https://github.com/dlmarion/accumulo/commit/d514b7be3e6924ed23a618e36657989f9eaf340b). Running AccumuloClientIT on the command line yeilds:
>
> ```
> java.lang.AssertionError: expected:<CLIENT> but was:<SERVER>
> at org.junit.Assert.fail(Assert.java:89)
> at org.junit.Assert.failNotEquals(Assert.java:835)
> at org.junit.Assert.assertEquals(Assert.java:120)
> at org.junit.Assert.assertEquals(Assert.java:146)
> at org.apache.accumulo.test.functional.AccumuloClientIT.testClose(AccumuloClientIT.java:191)
> ```
>
> I'm not sure what's going on here.
The call to `getServerContext` is the problematic part. You avoided it by removing my `getServerContext().getZooReaderWriter()` and `getServerContext().getZooKeeperRoot()`, but then you added it back in for `getServerContext().getConfiguration()` and `getServerContext().getInstanceID()`.
If the solution is to use ZooKeeper's API directly instead of our own, then we have to do it using the config for MiniAccumuloClusterImpl without instantiating the ServerContext because that's the part that sets the singleton mode to SERVER. This is timing based anyway... because for most tests, `getServerContext()` will eventually be called. We just can't have it called before `verifyUp()` because some of the tests check for the singleton mode (haven't looked into why).
--
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 pull request #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410#issuecomment-1009310709
> The call to getServerContext is the problematic part.
I just found that out by running a test in the debugger with a watch set on the SingletonManager.mode. I think I have the fix, buiding now and then going to kick off an IT run in Github Actions.
--
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 merged pull request #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410
--
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 #2410: Fix MiniAccumuloClusterImpl.verifyUp()
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2410:
URL: https://github.com/apache/accumulo/pull/2410#issuecomment-1007659778
@dlmarion Apparently, this fixed it for most of the tests, but there's at least one test that this breaks that I'll have to figure out: AccumuloClientIT.testClose asserts that we're in a CLIENT mode, but MiniAccumuloCluster specifically uses SERVER mode. The test passed before because mini lazily created a ServerContext that put it in SERVER mode. With these changes, the ServerContext is created sooner than it would have been.
There's a few other tests that failed as a result of my fix as well (not anywhere near the 139 that were failing before, though), but those could just be flakes. I'd have to investigate. Including the one I already mentioned, the list is:
```
org.apache.accumulo.test.functional.AccumuloClientIT.testClose
org.apache.accumulo.test.functional.CleanUpIT.run
org.apache.accumulo.test.functional.DurabilityIT.testIncreaseDurability
org.apache.accumulo.test.functional.LateLastContactIT.test
org.apache.accumulo.test.functional.ReadWriteIT.sunnyDay
org.apache.accumulo.test.functional.RestartIT.restartManagerRecovery
```
--
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