Fix #1084 partial - IT tests failing on Standalone by hkeebler · Pull Request #1164 · apache/accumulo

hkeebler · 2019-05-16T15:57:08Z

AccumuloClientIT, - fixed in AccumuloClusterHarness (avoid setting Servercontext) Will likely still fail if running SASL. The tests were not cleaning up the users so used more the scheme already in the harness but that could really be cleaned up even more for all tests and probably a separate ticket.
ManyWariteaheadLogsIT - Needed to alter standalone config.
ReadWriteIT, - reduced what was necessary to identify an SSL monitor and removed dependency on reading the props file.
YieldScannersIT, - adding a cleanup to the YieldingIterator
BadDeleteMarkers...IT and others that failed due to null in the command prefix - One solution is to always have an entry in the accumulo.it.properties file for server and client prefix but additionally added the cleanup in AccumuloClusterHarness to ensure a null is not passed.
For tests timing out I added a comment to the Testing.md to up the timeout.

TESTING.md

hkeebler · 2019-05-16T17:45:30Z

Also pushed up the IteratorEnvIT test change. The Asserts were failing on the standalone because junit is not deployed with accumulo. A RuntimeException was the only exception that did not require a "throws" or change to the implemented interface.

milleruntime · 2019-05-21T18:31:53Z

@hkeebler do you think this is ready to go? I will take a look at your changes to the ITs.

hkeebler · 2019-05-21T18:48:53Z

yes.

milleruntime · 2019-05-21T20:09:40Z

Are the ITs passing now while running standalone?

I am in the process of setting up a standalone on my machine to run your changes but found this: https://github.com/apache/fluo-uno/blob/master/conf/accumulo/common/accumulo-it.properties
Which should be useful for this testing. I think @mikewalch added this last year to uno.

hkeebler · 2019-05-21T20:24:54Z

Yes. I am running with it and have added (per issue description)
accumulo.it.cluster.standalone.users.1=user1
accumulo.it.cluster.standalone.passwords.1=user1
accumulo.it.cluster.standalone.keytab.1=
accumulo.it.cluster.standalone.users.2=user2
accumulo.it.cluster.standalone.passwords.2=user2
accumulo.it.cluster.standalone.users.3=user3
accumulo.it.cluster.standalone.passwords.3=user3
accumulo.it.cluster.standalone.users.4=user4
accumulo.it.cluster.standalone.passwords.4=user4
accumulo.it.cluster.standalone.users.5=user5
accumulo.it.cluster.standalone.passwords.5=user5
accumulo.it.cluster.standalone.users.6=user6
accumulo.it.cluster.standalone.passwords.6=user6

Also a fix for the "cmd - null entry" errors is to add
#accumulo.it.cluster.standalone.client.cmd.prefix=
#accumulo.it.cluster.standalone.server.cmd.prefix=
BUT I have now commented them out to test the change I put in -re: AccumuloClusterHarness

milleruntime

This is looking good so far. I just have a few comments and questions. I also saw this error when I setup an uno instance locally and ran the maven command in my comment.

[INFO] Running org.apache.accumulo.test.functional.AccumuloClientIT
[ERROR] Tests run: 3, Failures: 0, Errors: 3, Skipped: 0, Time elapsed: 2.312 s <<< FAILURE! - in org.apache.accumulo.test.functional.AccumuloClientIT
[ERROR] testClose(org.apache.accumulo.test.functional.AccumuloClientIT)  Time elapsed: 0.842 s  <<< ERROR!
java.lang.IllegalArgumentException: Invalid offset, should be non-negative and less than 0
	at org.apache.accumulo.test.functional.AccumuloClientIT.deleteUsers(AccumuloClientIT.java:56)

I plan on running the rest of your changes tomorrow.

test/src/main/java/org/apache/accumulo/harness/AccumuloClusterHarness.java

milleruntime · 2019-05-21T20:29:54Z

test/src/main/java/org/apache/accumulo/test/functional/ManyWriteAheadLogsIT.java

+      getClusterControl().stopAllServers(ServerType.TABLET_SERVER);
+      getClusterControl().startAllServers(ServerType.TABLET_SERVER);
+    }
+  }


Could you just put this in configureMiniCluster()? Or does it have to be before configureMiniCluster is called?

This is an interesting solution for the standalone case. However, if it is necessary to alter the configuration of the system for the test, then it is probably not suitable for a standalone test, and should be recategorized so as to be skipped when running against a standalone instance.

@milleruntime configureMiniCluster() is called when a MAC is used. In this case we are running standalone so it would not be called.

test/src/main/java/org/apache/accumulo/test/functional/ManyWriteAheadLogsIT.java

test/src/main/java/org/apache/accumulo/test/functional/AccumuloClientIT.java

keith-turner · 2019-05-22T17:23:44Z

test/src/main/java/org/apache/accumulo/test/functional/AccumuloClientIT.java

+    ClusterUser testuser1 = getUser(0);
+    final String user1 = testuser1.getPrincipal();
+    final String password1 = testuser1.getPassword();
+    c.securityOperations().createLocalUser(user1, new PasswordToken(password1));


The purpose of the ClusterUser abstraction is to allow test to work with passwords or kerberose. For this test to work with kerberose it should not use PasswordToken(), but instead use testuser1.getToken()

Thanks. I'll assume a kerberose check is needed.

Hopefully testuser1.getToken() is an easy change and it works with password or kerberose. If not this could be a follow on issue.

Nevermind, this is not as simple as I thought. I was thinking c.securityOperations().createLocalUser(user1, testuser1.getToken()). However I looked at the method createLocalUser and it explicitly takes a PasswordToken. I saw some other test code passes null for the token when sasl is enabled, but I Am not sure what the correct thing to do.

keith-turner · 2019-05-22T17:25:08Z

test/src/main/java/org/apache/accumulo/test/functional/AccumuloClientIT.java


 public class AccumuloClientIT extends AccumuloClusterHarness {

+  @Before


It seems like it would be better to make this method an @After method.

I agree. I finally saw the error, the second time I ran the test:
org.apache.accumulo.core.client.AccumuloSecurityException: Error USER_EXISTS for user testuser - The user exists
But it would make more sense to put it as an @After method.

agree. I'll test it out. BUT what if we updated the harness to create users prefixed with the methodname (like the tables) - then the deleteusers in the harness would work. Is there a limit to the number of characters a username can have?

what if we updated the harness to create users prefixed with the methodname (like the tables)

That may make sense to do. That feels like a follow on issue, what do you think?

The class name as a prefix is confusing because with inheritance and multiple classes its hard to know if you got it right. Maybe just use a constant like

public static final String HARNESS_USER_PREFIX="a1b1ae6bfef2ca11d92b86ab60930b3c56971f6e"

Then all code can just use that constant. I picked the latest commit hash on the master branch for the value. The constant could also be a classname, it does not really matter what it is as long its somewhat unique.

So I see this request is accepted. Do you still want me to move the deleteuser to the @after? Or did someone do that already?

I think it would be good to make it after

hkeebler · 2019-05-22T17:55:56Z

I'll create a ticket to update the user creation , deletion in the harness. thanks.

keith-turner · 2019-05-22T17:56:02Z

apache/accumulo-fluo-uno#223

milleruntime · 2019-05-22T18:03:08Z

OK I will merge this PR as I think its ready to go. @hkeebler What would you like the GitHub comment to say?

keith-turner · 2019-05-22T18:08:11Z

OK I will merge this PR as I think its ready to go. @hkeebler What would you like the GitHub comment to say?

It would be nice to make the @After and getToken() changes.

hkeebler · 2019-05-22T18:10:30Z

You don't like the one that is there? Fix #1084 Fixes many of the IT tests failing on standalone cluster

hkeebler · 2019-05-22T18:11:42Z

I'll make those changes now and repush so wait

milleruntime · 2019-05-22T18:58:49Z

You don't like the one that is there? Fix #1084 Fixes many of the IT tests failing on standalone cluster

This is what shows up on the command line:
"Fix #1084 partial - IT tests failing on Standalone AccumuloClientIT, ManyWariteaheadLogsIT, ReadWriteIT, YieldScannersIT, BadDeleteMarkers...IT and others that failed due to null in the command prefix Also includes IteratorEnvIT fix"

AccumuloClientIT, ManyWariteaheadLogsIT, ReadWriteIT, YieldScannersIT, BadDeleteMarkers...IT and others that failed due to null in the command prefix Also includes IteratorEnvIT fix

hkeebler · 2019-05-22T19:26:07Z

@keith-turner I made the @after change and comment cleanup change. I did not make the getToken() for kerberose change. The test has alot going on and I am unfamiliar with what else would need to be updated for sasl or kerberose. If desired we can take this test out of this set of updates.

keith-turner · 2019-05-22T19:52:02Z

The test has alot going on and I am unfamiliar with what else would need to be updated for sasl or kerberose. If desired we can take this test out of this set of updates.

@hkeebler just making the @after changes is fine. I am not sure how best to use the getToken() method in this case after taking a closer look at the code.

milleruntime · 2019-06-04T19:14:05Z

@hkeebler is this PR finished?

hkeebler · 2019-06-04T19:14:52Z

yes, you approved it a while back

hkeebler · 2019-06-04T19:16:32Z

sorry, use the hkeebler:accumulo-1084 commit. I mistakenly created a pull request instead of recommitting initially.

hkeebler · 2019-06-04T19:21:43Z

@milleruntime I did not intend to close this. Did I do that or did you do that?

milleruntime · 2019-06-04T19:21:56Z

sorry, use the hkeebler:accumulo-1084 commit. I mistakenly created a pull request instead of recommitting initially.

I am confused...you closed this PR. Did you still want me to merge it?

hkeebler · 2019-06-04T19:23:20Z

I'm confused. Yes I wanted it to be merged. Oh I see, Imust have clicked the button on the left.

milleruntime · 2019-06-04T19:27:46Z

I'm confused. Yes I wanted it to be merged. Oh I see, Imust have clicked the button on the left.

Yes, probably. OK I am merging this now.

ctubbsii reviewed May 16, 2019

View reviewed changes

TESTING.md Outdated Show resolved Hide resolved

hkeebler force-pushed the accumulo-1084 branch from 46f47f4 to eae21cc Compare May 16, 2019 17:05

hkeebler force-pushed the accumulo-1084 branch from eae21cc to be09e3a Compare May 20, 2019 17:06

milleruntime reviewed May 21, 2019

View reviewed changes

milleruntime reviewed May 22, 2019

View reviewed changes

test/src/main/java/org/apache/accumulo/test/functional/AccumuloClientIT.java Show resolved Hide resolved

keith-turner reviewed May 22, 2019

View reviewed changes

milleruntime approved these changes May 22, 2019

View reviewed changes

Fix apache#1084 partial - IT tests failing on Standalone

d77962c

AccumuloClientIT, ManyWariteaheadLogsIT, ReadWriteIT, YieldScannersIT, BadDeleteMarkers...IT and others that failed due to null in the command prefix Also includes IteratorEnvIT fix

hkeebler force-pushed the accumulo-1084 branch from be09e3a to d77962c Compare May 22, 2019 19:22

hkeebler mentioned this pull request May 23, 2019

Clean up user/table creation/cleanup in AccumuloClusterHarness #1171

Closed

ctubbsii changed the base branch from master to 2.0 May 28, 2019 18:00

ctubbsii added the v2.0.0 label May 28, 2019

hkeebler closed this Jun 4, 2019

milleruntime reopened this Jun 4, 2019

milleruntime merged commit fcdbc0b into apache:2.0 Jun 4, 2019

hkeebler deleted the accumulo-1084 branch July 8, 2019 19:03

ctubbsii added this to the 2.0.0 milestone Jul 12, 2024


		public class AccumuloClientIT extends AccumuloClusterHarness {

		@Before

Conversation

hkeebler commented May 16, 2019

Uh oh!

Uh oh!

hkeebler commented May 16, 2019

Uh oh!

milleruntime commented May 21, 2019

Uh oh!

hkeebler commented May 21, 2019

Uh oh!

milleruntime commented May 21, 2019

Uh oh!

hkeebler commented May 21, 2019

Uh oh!

milleruntime left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkeebler commented May 22, 2019

Uh oh!

keith-turner commented May 22, 2019

Uh oh!

milleruntime commented May 22, 2019

Uh oh!

keith-turner commented May 22, 2019

Uh oh!

hkeebler commented May 22, 2019

Uh oh!

hkeebler commented May 22, 2019

Uh oh!

milleruntime commented May 22, 2019

Uh oh!

hkeebler commented May 22, 2019

Uh oh!

keith-turner commented May 22, 2019

Uh oh!

milleruntime commented Jun 4, 2019

Uh oh!

hkeebler commented Jun 4, 2019

Uh oh!

hkeebler commented Jun 4, 2019

Uh oh!

hkeebler commented Jun 4, 2019

Uh oh!

milleruntime commented Jun 4, 2019

Uh oh!

hkeebler commented Jun 4, 2019

Uh oh!

milleruntime commented Jun 4, 2019

Uh oh!

keith-turner May 22, 2019 •

edited

Loading