You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by joshelser <gi...@git.apache.org> on 2015/11/09 16:53:34 UTC
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
GitHub user joshelser opened a pull request:
https://github.com/apache/accumulo/pull/50
ACCUMULO-626 Iterator fuzzer/tester
Some early code for automatically testing iterators.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/joshelser/accumulo ACCUMULO-626-fuzzer
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/50.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #50
----
commit 768a118c492229463df5c8c361c33559d92b7508
Author: Josh Elser <el...@apache.org>
Date: 2015-11-07T19:17:17Z
ACCUMULO-626 Initial outline of the test harness.
commit fecf28ae30e845457e62a13efca5be83b4fc04ef
Author: Josh Elser <el...@apache.org>
Date: 2015-11-07T21:09:17Z
Create a basic JUnit4 test case that wraps IteratorTestRunner.
commit 21b3ce14dc4199a3b0974d33ac04b0767543554d
Author: Josh Elser <el...@apache.org>
Date: 2015-11-07T22:22:13Z
Some initial tests against WholeRowIterator
commit 11bf87f8b13a45f82f04a5f6617e55fc0af4dea5
Author: Josh Elser <el...@apache.org>
Date: 2015-11-07T22:31:58Z
Add a base TestCase that performs verification on the TestOutput.
commit 2f71336d683c94106997a2e2ed2c2471c8c6b707
Author: Josh Elser <el...@apache.org>
Date: 2015-11-07T22:52:30Z
Add a utility to dynamically find all provided IteratorTestCases.
commit b00a1b794b1d0787cdb43185b917b5e7b7b5cff0
Author: Josh Elser <el...@apache.org>
Date: 2015-11-08T08:03:44Z
DeepCopy and ReSeek test cases.
commit 273f1f5c6e026a2bc0810e9f5bf22a7c98bf9e13
Author: Josh Elser <el...@apache.org>
Date: 2015-11-08T08:05:57Z
More docs.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-163742584
> Well currently iterators are not declared as public API.
Ooo, that is a good point (which I often forget). Perhaps I'll just table the idea for now and we can address if/when we do something with iterators.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-155167375
For iterators that can be run at major compaction, how can we exercise delete keys?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-163845209
Okie doke. Pulled in Dylan's test improvements (thanks again for that!). I think the only thing I have left that I want to do is to wire up as many of the existing iterators we provide for users with some tests in the accumulo-test module.
Any concerns, speak up soon. I'll likely merge over the weekend unless I hear objection to not do so. Then, I will file some follow-on JIRA work for the lingering improvements.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-163673419
Drat, another thing I just thought of: this is meant to be user facing, and I think it should eventually hit public API. It might be premature to put it there immediately (since this hasn't been vetted by anyone but myself), but does anyone have thoughts on a path to "graduate" into public API status?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r47255171
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/testcases/DeepCopyTestCase.java ---
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest.testcases;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.iteratortest.IteratorTestInput;
+import org.apache.accumulo.iteratortest.IteratorTestOutput;
+import org.apache.accumulo.iteratortest.IteratorTestUtil;
+import org.apache.accumulo.iteratortest.environments.SimpleIteratorEnvironment;
+
+/**
+ * Test case that verifies that an iterator can use the generated instance from {@code deepCopy}.
+ */
+public class DeepCopyTestCase extends OutputVerifyingTestCase {
+
+ @Override
+ public IteratorTestOutput test(IteratorTestInput testInput) {
+ final SortedKeyValueIterator<Key,Value> skvi = IteratorTestUtil.instantiateIterator(testInput);
+ final SortedKeyValueIterator<Key,Value> source = IteratorTestUtil.createSource(testInput);
+
+ try {
+ skvi.init(source, testInput.getIteratorOptions(), new SimpleIteratorEnvironment());
+
+ SortedKeyValueIterator<Key,Value> copy = skvi.deepCopy(new SimpleIteratorEnvironment());
--- End diff --
Thanks for taking a look, Dylan!
> call deepCopy in the middle of an iteration, in addition to the beginning after init() but before any next() calls
Yeah, there are lots of good tests we can do around deepCopy. I'll see if I can add one for this specific case. Please feel free to send a PR if you definitely want to see this in the initial changeset :)
> Some iterators may do this in order to "save" the iteration at a certain point.
But that's not something the "system" does, is it? I can see the value, but I'm not sure if that's something that would happen through "normal" Accumulo use.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-155168971
> For iterators that can be run at major compaction, how can we exercise delete keys?
Very good question! The completely mocked-out IteratorEnvironment is a sign that I haven't thought past the scan case. I'm not even sure if the ColumnFiltering iterator with SortedMapIterator is even a 100% valid in-memory reproduction of how it works in a TabletServer.
I had left input entirely up to the user, so I'm not sure of a simple way to test deletions without potentially messing up some expected table structure. There are some tricky edge cases at compaction that I haven't yet considered how to encapsulate. Combiners on non-full compactions are another good example.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-163484617
Rebased on top of master and addressed the current comments:
* Removed Reflections dependency
* Reuse Key/Value in the test iterator stack
* New deepCopy test that iterates over multiple copies.
* More accurate re-seek test case.
Only outstanding thing I want to do before an initial commit is to run all of the iterators in {{o.a.a.c.iterators}} through this framework in accumulo-test (start the dog-fooding).
Keith had some suggestions about fancier testing (iterator stacks, compaction/scopes, and delete keys), which I'm not sure I want to try to solve right away. I feel like I've let these changes sit for far too long already. I'd be happy to wait if anyone else is interested in working on it short-term, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44318310
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/testcases/ReSeekTestCase.java ---
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest.testcases;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Random;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.iteratortest.IteratorTestInput;
+import org.apache.accumulo.iteratortest.IteratorTestOutput;
+import org.apache.accumulo.iteratortest.IteratorTestUtil;
+import org.apache.accumulo.iteratortest.environments.SimpleIteratorEnvironment;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Test case that verifies that an iterator can use the generated instance from {@code deepCopy}.
+ */
+public class ReSeekTestCase extends OutputVerifyingTestCase {
+ private static final Logger log = LoggerFactory.getLogger(ReSeekTestCase.class);
+
+ /**
+ * Let N be a random number between [0, RESEEK_INTERVAL). After every Nth entry "returned" to the client, recreate and reseek the iterator.
+ */
+ private static final int RESEEK_INTERVAL = 4;
+
+ private final Random random;
+
+ public ReSeekTestCase() {
+ this.random = new Random();
+ }
+
+ @Override
+ public IteratorTestOutput test(IteratorTestInput testInput) {
+ final SortedKeyValueIterator<Key,Value> skvi = IteratorTestUtil.instantiateIterator(testInput);
+ final SortedKeyValueIterator<Key,Value> source = IteratorTestUtil.createSource(testInput);
+
+ try {
+ skvi.init(source, testInput.getIteratorOptions(), new SimpleIteratorEnvironment());
+ skvi.seek(testInput.getRange(), Collections.<ByteSequence> emptySet(), false);
+ return new IteratorTestOutput(consume(skvi, testInput));
+ } catch (IOException e) {
+ return new IteratorTestOutput(e);
+ }
+ }
+
+ TreeMap<Key,Value> consume(SortedKeyValueIterator<Key,Value> skvi, IteratorTestInput testInput) throws IOException {
+ final TreeMap<Key,Value> data = new TreeMap<>();
+ final Range origRange = testInput.getRange();
+ int reseekCount = random.nextInt(RESEEK_INTERVAL);
+
+ int i = 0;
+ while (skvi.hasTop()) {
+ data.put(skvi.getTopKey(), skvi.getTopValue());
+
+ /*
+ * One of the trickiest cases in writing iterators:
+ *
+ * After any result is returned from a TabletServer to the client, the Iterator in the TabletServer's memory may be torn down. To preserve the state and
+ * guarantee that all records are received, the TabletServer does remember the last Key it returned to the client. It will recreate the Iterator (stack),
+ * and seek it using an updated Range. This range's start key is set to the last Key returned, non-inclusive.
+ */
+ if (i % RESEEK_INTERVAL == reseekCount) {
+ // Last key
+ Key reSeekStartKey = skvi.getTopKey();
+
+ // Deepcopy the iterator
+ skvi = skvi.deepCopy(new SimpleIteratorEnvironment());
--- End diff --
creating a completely new iterator instance and initializing it would be more realistic
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44322387
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/testcases/DeepCopyTestCase.java ---
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest.testcases;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.iteratortest.IteratorTestInput;
+import org.apache.accumulo.iteratortest.IteratorTestOutput;
+import org.apache.accumulo.iteratortest.IteratorTestUtil;
+import org.apache.accumulo.iteratortest.environments.SimpleIteratorEnvironment;
+
+/**
+ * Test case that verifies that an iterator can use the generated instance from {@code deepCopy}.
+ */
+public class DeepCopyTestCase extends OutputVerifyingTestCase {
+
+ @Override
+ public IteratorTestOutput test(IteratorTestInput testInput) {
+ final SortedKeyValueIterator<Key,Value> skvi = IteratorTestUtil.instantiateIterator(testInput);
+ final SortedKeyValueIterator<Key,Value> source = IteratorTestUtil.createSource(testInput);
+
+ try {
+ skvi.init(source, testInput.getIteratorOptions(), new SimpleIteratorEnvironment());
+
+ SortedKeyValueIterator<Key,Value> copy = skvi.deepCopy(new SimpleIteratorEnvironment());
+
+ copy.seek(testInput.getRange(), Collections.<ByteSequence> emptySet(), false);
--- End diff --
Seeking one deep copy should have no impact on other deep copies. Would be nice to verify this. Could interleave reading and seeking a few deep copies.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/50
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44370042
--- Diff: iterator-test-harness/pom.xml ---
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-project</artifactId>
+ <version>1.8.0-SNAPSHOT</version>
+ </parent>
+ <artifactId>accumulo-iterator-test-harness</artifactId>
+ <name>Apache Accumulo Iterator Test Harness</name>
+ <description>A library for testing Apache Accumulo Iterators.</description>
+ <dependencies>
+ <!-- TODO Don't force downstream users to have JUnit -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <!--optional>true</optional-->
+ </dependency>
+ <dependency>
+ <groupId>log4j</groupId>
+ <artifactId>log4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.reflections</groupId>
+ <artifactId>reflections</artifactId>
+ </dependency>
--- End diff --
> That license even permits the code to be freely re-licensed, so even if we did include it, we could include it under the Apache License instead anyway.
This came up in LEGAL-135 and the community felt it was better to leave as is, instead of re-licensing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by dhutchis <gi...@git.apache.org>.
Github user dhutchis commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r47256202
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/testcases/DeepCopyTestCase.java ---
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest.testcases;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.iteratortest.IteratorTestInput;
+import org.apache.accumulo.iteratortest.IteratorTestOutput;
+import org.apache.accumulo.iteratortest.IteratorTestUtil;
+import org.apache.accumulo.iteratortest.environments.SimpleIteratorEnvironment;
+
+/**
+ * Test case that verifies that an iterator can use the generated instance from {@code deepCopy}.
+ */
+public class DeepCopyTestCase extends OutputVerifyingTestCase {
+
+ @Override
+ public IteratorTestOutput test(IteratorTestInput testInput) {
+ final SortedKeyValueIterator<Key,Value> skvi = IteratorTestUtil.instantiateIterator(testInput);
+ final SortedKeyValueIterator<Key,Value> source = IteratorTestUtil.createSource(testInput);
+
+ try {
+ skvi.init(source, testInput.getIteratorOptions(), new SimpleIteratorEnvironment());
+
+ SortedKeyValueIterator<Key,Value> copy = skvi.deepCopy(new SimpleIteratorEnvironment());
--- End diff --
Ah I see Josh-- is the purpose to test only system iterators, as opposed to generic tests to check user-defined iterators? That makes more sense.
Some users may define iterators that expect a column family, whereas all the tests seek with `Collections.<ByteSequence> emptySet(), false`. There may be many other places where a user does something "non-normal" intentionally. If we make this public API, we should state that user iterators do not necessarily need to pass all the IteratorTestCases, or at least that failing an IteratorTestCase does not imply a bug.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44311672
--- Diff: iterator-test-harness/src/test/java/org/apache/accumulo/iteratortest/WholeRowIteratorTest.java ---
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map.Entry;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.user.WholeRowIterator;
+import org.apache.accumulo.iteratortest.junit4.BaseJUnit4IteratorTest;
+import org.apache.accumulo.iteratortest.testcases.IteratorTestCase;
+import org.apache.hadoop.io.Text;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * Framework tests for {@link WholeRowIterator}.
+ */
+public class WholeRowIteratorTest extends BaseJUnit4IteratorTest {
+
+ @Parameters
+ public static Object[][] parameters() {
+ IteratorTestInput input = getIteratorInput();
+ IteratorTestOutput output = getIteratorOutput();
+ List<IteratorTestCase> tests = IteratorTestCaseFinder.findAllTestCases();
+ return BaseJUnit4IteratorTest.createParameters(input, output, tests);
+ }
+
+ private static final TreeMap<Key,Value> INPUT_DATA = createInputData();
+ private static final TreeMap<Key,Value> OUTPUT_DATA = createOutputData();
+
+ private static TreeMap<Key,Value> createInputData() {
+ TreeMap<Key,Value> data = new TreeMap<>();
+
+ data.put(new Key("1", "", "a"), new Value("1a".getBytes()));
+ data.put(new Key("1", "", "b"), new Value("1b".getBytes()));
+ data.put(new Key("1", "a", "a"), new Value("1aa".getBytes()));
+ data.put(new Key("1", "a", "b"), new Value("1ab".getBytes()));
+ data.put(new Key("1", "b", "a"), new Value("1ba".getBytes()));
+
+ data.put(new Key("2", "a", "a"), new Value("2aa".getBytes()));
+ data.put(new Key("2", "a", "b"), new Value("2ab".getBytes()));
+ data.put(new Key("2", "a", "c"), new Value("2ac".getBytes()));
+ data.put(new Key("2", "c", "c"), new Value("2cc".getBytes()));
+
+ data.put(new Key("3", "a", ""), new Value("3a".getBytes()));
+
+ data.put(new Key("4", "a", "b"), new Value("4ab".getBytes()));
+
+ data.put(new Key("5", "a", "a"), new Value("5aa".getBytes()));
+ data.put(new Key("5", "a", "b"), new Value("5ab".getBytes()));
+ data.put(new Key("5", "a", "c"), new Value("5ac".getBytes()));
+ data.put(new Key("5", "a", "d"), new Value("5ad".getBytes()));
+
+ data.put(new Key("6", "", "a"), new Value("6a".getBytes()));
+ data.put(new Key("6", "", "b"), new Value("6b".getBytes()));
+ data.put(new Key("6", "", "c"), new Value("6c".getBytes()));
+ data.put(new Key("6", "", "d"), new Value("6d".getBytes()));
+ data.put(new Key("6", "", "e"), new Value("6e".getBytes()));
+ data.put(new Key("6", "1", "a"), new Value("61a".getBytes()));
+ data.put(new Key("6", "1", "b"), new Value("61b".getBytes()));
+ data.put(new Key("6", "1", "c"), new Value("61c".getBytes()));
+ data.put(new Key("6", "1", "d"), new Value("61d".getBytes()));
+ data.put(new Key("6", "1", "e"), new Value("61e".getBytes()));
+
+ return data;
+ }
+
+ private static TreeMap<Key,Value> createOutputData() {
+ TreeMap<Key,Value> data = new TreeMap<>();
+
+ Text row = null;
+ List<Key> keys = new ArrayList<>();
+ List<Value> values = new ArrayList<>();
+
+ // Generate the output data from the input data
+ for (Entry<Key,Value> entry : INPUT_DATA.entrySet()) {
--- End diff --
Using [RowIterator](https://accumulo.apache.org/1.6/apidocs/org/apache/accumulo/core/client/RowIterator.html) may simplify this code a bit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44312491
--- Diff: iterator-test-harness/pom.xml ---
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-project</artifactId>
+ <version>1.8.0-SNAPSHOT</version>
+ </parent>
+ <artifactId>accumulo-iterator-test-harness</artifactId>
+ <name>Apache Accumulo Iterator Test Harness</name>
+ <description>A library for testing Apache Accumulo Iterators.</description>
+ <dependencies>
+ <!-- TODO Don't force downstream users to have JUnit -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <!--optional>true</optional-->
+ </dependency>
+ <dependency>
+ <groupId>log4j</groupId>
+ <artifactId>log4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.reflections</groupId>
+ <artifactId>reflections</artifactId>
+ </dependency>
--- End diff --
No worries, your attention to that detail is appreciated. It was really just a "google, copy/paste" job to keep me moving on that I didn't mean to keep long-term. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44316155
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/IteratorTestUtil.java ---
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.iterators.SortedMapIterator;
+import org.apache.accumulo.core.iterators.system.ColumnFamilySkippingIterator;
+import org.apache.accumulo.iteratortest.testcases.IteratorTestCase;
+
+/**
+ * A collection of methods that are helpful to the development of {@link IteratorTestCase}s.
+ */
+public class IteratorTestUtil {
+
+ public static SortedKeyValueIterator<Key,Value> instantiateIterator(IteratorTestInput input) {
+ try {
+ return Objects.requireNonNull(input.getIteratorClass()).newInstance();
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static SortedKeyValueIterator<Key,Value> createSource(IteratorTestInput input) {
+ return new ColumnFamilySkippingIterator(new SortedMapIterator(Objects.requireNonNull(input).getInput()));
--- End diff --
b. loss did in this in TransformingIteratorTest. See line [#662 in 1.6.4](https://github.com/apache/accumulo/blob/1.6.4/core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java#L662)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44310769
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/IteratorTestCaseFinder.java ---
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest;
+
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.accumulo.iteratortest.testcases.IteratorTestCase;
+import org.reflections.Reflections;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A class to ease finding published test cases.
+ */
+public class IteratorTestCaseFinder {
+ private static final Logger log = LoggerFactory.getLogger(IteratorTestCaseFinder.class);
+
+ /**
+ * Instantiates all test cases provided.
+ *
+ * @return A list of {@link IteratorTestCase}s.
+ */
+ public static List<IteratorTestCase> findAllTestCases() {
+ log.info("Searching {}", IteratorTestCase.class.getPackage().getName());
+ Reflections reflections = new Reflections(IteratorTestCase.class.getPackage().getName());
--- End diff --
If this is a new dependency, maybe you could use Guava's [ClassPath.getAllClasses()][1]. Could iterator an filter based on type.
[1]: http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/reflect/ClassPath.html#getAllClasses%28%29
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44291969
--- Diff: iterator-test-harness/pom.xml ---
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-project</artifactId>
+ <version>1.8.0-SNAPSHOT</version>
+ </parent>
+ <artifactId>accumulo-iterator-test-harness</artifactId>
+ <name>Apache Accumulo Iterator Test Harness</name>
+ <description>A library for testing Apache Accumulo Iterators.</description>
+ <dependencies>
+ <!-- TODO Don't force downstream users to have JUnit -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <!--optional>true</optional-->
+ </dependency>
+ <dependency>
+ <groupId>log4j</groupId>
+ <artifactId>log4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.reflections</groupId>
+ <artifactId>reflections</artifactId>
+ </dependency>
--- End diff --
uhhh license is http://www.wtfpl.net/. Ideally, this will be ripped out before it's committed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by wjsl <gi...@git.apache.org>.
Github user wjsl commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44312285
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/IteratorTestUtil.java ---
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.iterators.SortedMapIterator;
+import org.apache.accumulo.core.iterators.system.ColumnFamilySkippingIterator;
+import org.apache.accumulo.iteratortest.testcases.IteratorTestCase;
+
+/**
+ * A collection of methods that are helpful to the development of {@link IteratorTestCase}s.
+ */
+public class IteratorTestUtil {
+
+ public static SortedKeyValueIterator<Key,Value> instantiateIterator(IteratorTestInput input) {
+ try {
+ return Objects.requireNonNull(input.getIteratorClass()).newInstance();
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static SortedKeyValueIterator<Key,Value> createSource(IteratorTestInput input) {
+ return new ColumnFamilySkippingIterator(new SortedMapIterator(Objects.requireNonNull(input).getInput()));
--- End diff --
Might be good to make another iterator here (or wrapping the SMI) that re-uses the Key and Value instances returned by the getters. Storing the output of a source in a list or map without making a copy is a common pitfall.
I had a small iterator benchmark that I modified to printout the identify of each Key and Value, and currently notice the same Value instance has the potential to be re-used. I will try to verify if Keys exhibit the same behavior-- I have heard anecdotes that they do.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-155165730
There may be value in running some of these test against an iterator stack and not just a single iterator.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r47257077
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/testcases/DeepCopyTestCase.java ---
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest.testcases;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.iteratortest.IteratorTestInput;
+import org.apache.accumulo.iteratortest.IteratorTestOutput;
+import org.apache.accumulo.iteratortest.IteratorTestUtil;
+import org.apache.accumulo.iteratortest.environments.SimpleIteratorEnvironment;
+
+/**
+ * Test case that verifies that an iterator can use the generated instance from {@code deepCopy}.
+ */
+public class DeepCopyTestCase extends OutputVerifyingTestCase {
+
+ @Override
+ public IteratorTestOutput test(IteratorTestInput testInput) {
+ final SortedKeyValueIterator<Key,Value> skvi = IteratorTestUtil.instantiateIterator(testInput);
+ final SortedKeyValueIterator<Key,Value> source = IteratorTestUtil.createSource(testInput);
+
+ try {
+ skvi.init(source, testInput.getIteratorOptions(), new SimpleIteratorEnvironment());
+
+ SortedKeyValueIterator<Key,Value> copy = skvi.deepCopy(new SimpleIteratorEnvironment());
--- End diff --
> is the purpose to test only system iterators, as opposed to generic tests to check user-defined iterators? That makes more sense.
Yep. First and foremost, my goal was to provide a framework for users to test iterators without running them through Accumulo (since it's often hard to know when you're hitting some of the edge cases). However, I can totally see it growing into a framework to catch "common pitfalls" like you outlined :)
> Some users may define iterators that expect a column family, whereas all the tests seek with Collections.<ByteSequence> emptySet(), false
Yeah, this is something I'm not quite sure how to handle colfams (I'm throwing that into the same bucket as the compaction stuff and more that Keith pointed out).
> If we make this public API, we should state that user iterators do not necessarily need to pass all the IteratorTestCases, or at least that failing an IteratorTestCase does not imply a bug.
Very good point. We don't really have an "evolving" annotation. Maybe we just talk about it and the lack of public API declares it as evolving intrinsically?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44311676
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/IteratorTestCaseFinder.java ---
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest;
+
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.accumulo.iteratortest.testcases.IteratorTestCase;
+import org.reflections.Reflections;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A class to ease finding published test cases.
+ */
+public class IteratorTestCaseFinder {
+ private static final Logger log = LoggerFactory.getLogger(IteratorTestCaseFinder.class);
+
+ /**
+ * Instantiates all test cases provided.
+ *
+ * @return A list of {@link IteratorTestCase}s.
+ */
+ public static List<IteratorTestCase> findAllTestCases() {
+ log.info("Searching {}", IteratorTestCase.class.getPackage().getName());
+ Reflections reflections = new Reflections(IteratorTestCase.class.getPackage().getName());
--- End diff --
Yeah, like I hinted at above, this was a quick hack to get this functionality and I hope to remove it (despite being a very nice library to use). Ideally, I'd not need any extra dependencies to do it (currently don't have any deps outside of accumulo-core, junit and slf4j that I recall), and I'd prefer to leave it that way. If it's hard to do with regular reflection libs, Guava sounds like a nice choice. Thanks for the recommendation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44330310
--- Diff: iterator-test-harness/pom.xml ---
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-project</artifactId>
+ <version>1.8.0-SNAPSHOT</version>
+ </parent>
+ <artifactId>accumulo-iterator-test-harness</artifactId>
+ <name>Apache Accumulo Iterator Test Harness</name>
+ <description>A library for testing Apache Accumulo Iterators.</description>
+ <dependencies>
+ <!-- TODO Don't force downstream users to have JUnit -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <!--optional>true</optional-->
+ </dependency>
+ <dependency>
+ <groupId>log4j</groupId>
+ <artifactId>log4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.reflections</groupId>
+ <artifactId>reflections</artifactId>
+ </dependency>
--- End diff --
LICENSE/NOTICE include things that we distribute, not things which satisfy our dependencies. So, no, I don't think we need to include anything extra there, unless we're including it's source in our project, or expecting to bundle its jar in our -bin.tar.gz
As for the WTFPL, it's fine... I remember looking into this a bit for [ACCUMULO-1496](https://issues.apache.org/jira/browse/ACCUMULO-1496). That license even permits the code to be freely re-licensed, so even if we did include it, we could include it under the Apache License instead anyway.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44316590
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/IteratorTestUtil.java ---
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.iterators.SortedMapIterator;
+import org.apache.accumulo.core.iterators.system.ColumnFamilySkippingIterator;
+import org.apache.accumulo.iteratortest.testcases.IteratorTestCase;
+
+/**
+ * A collection of methods that are helpful to the development of {@link IteratorTestCase}s.
+ */
+public class IteratorTestUtil {
+
+ public static SortedKeyValueIterator<Key,Value> instantiateIterator(IteratorTestInput input) {
+ try {
+ return Objects.requireNonNull(input.getIteratorClass()).newInstance();
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static SortedKeyValueIterator<Key,Value> createSource(IteratorTestInput input) {
+ return new ColumnFamilySkippingIterator(new SortedMapIterator(Objects.requireNonNull(input).getInput()));
--- End diff --
> Storing the output of a source in a list or map without making a copy is a common pitfall.
Ooo, that's a good one I didn't think to test. Sounds like a good idea to implicitly test. I can also see a test that checks that Keys and values aren't reused (with a reference check).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44312116
--- Diff: iterator-test-harness/pom.xml ---
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-project</artifactId>
+ <version>1.8.0-SNAPSHOT</version>
+ </parent>
+ <artifactId>accumulo-iterator-test-harness</artifactId>
+ <name>Apache Accumulo Iterator Test Harness</name>
+ <description>A library for testing Apache Accumulo Iterators.</description>
+ <dependencies>
+ <!-- TODO Don't force downstream users to have JUnit -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <!--optional>true</optional-->
+ </dependency>
+ <dependency>
+ <groupId>log4j</groupId>
+ <artifactId>log4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.reflections</groupId>
+ <artifactId>reflections</artifactId>
+ </dependency>
--- End diff --
LEGAL-135 and http://www.apache.org/legal/resolved.html#category-a indicate that it's fine. I just wanted to know if we need to document the new dependency somehow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by dhutchis <gi...@git.apache.org>.
Github user dhutchis commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r47254752
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/testcases/DeepCopyTestCase.java ---
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest.testcases;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.iteratortest.IteratorTestInput;
+import org.apache.accumulo.iteratortest.IteratorTestOutput;
+import org.apache.accumulo.iteratortest.IteratorTestUtil;
+import org.apache.accumulo.iteratortest.environments.SimpleIteratorEnvironment;
+
+/**
+ * Test case that verifies that an iterator can use the generated instance from {@code deepCopy}.
+ */
+public class DeepCopyTestCase extends OutputVerifyingTestCase {
+
+ @Override
+ public IteratorTestOutput test(IteratorTestInput testInput) {
+ final SortedKeyValueIterator<Key,Value> skvi = IteratorTestUtil.instantiateIterator(testInput);
+ final SortedKeyValueIterator<Key,Value> source = IteratorTestUtil.createSource(testInput);
+
+ try {
+ skvi.init(source, testInput.getIteratorOptions(), new SimpleIteratorEnvironment());
+
+ SortedKeyValueIterator<Key,Value> copy = skvi.deepCopy(new SimpleIteratorEnvironment());
--- End diff --
Some thoughts on another test-- call deepCopy in the middle of an iteration, in addition to the beginning after init() but before any next() calls. Some iterators may do this in order to "save" the iteration at a certain point.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by dhutchis <gi...@git.apache.org>.
Github user dhutchis commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r47257053
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/testcases/DeepCopyTestCase.java ---
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest.testcases;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.TreeMap;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.iteratortest.IteratorTestInput;
+import org.apache.accumulo.iteratortest.IteratorTestOutput;
+import org.apache.accumulo.iteratortest.IteratorTestUtil;
+import org.apache.accumulo.iteratortest.environments.SimpleIteratorEnvironment;
+
+/**
+ * Test case that verifies that an iterator can use the generated instance from {@code deepCopy}.
+ */
+public class DeepCopyTestCase extends OutputVerifyingTestCase {
+
+ @Override
+ public IteratorTestOutput test(IteratorTestInput testInput) {
+ final SortedKeyValueIterator<Key,Value> skvi = IteratorTestUtil.instantiateIterator(testInput);
+ final SortedKeyValueIterator<Key,Value> source = IteratorTestUtil.createSource(testInput);
+
+ try {
+ skvi.init(source, testInput.getIteratorOptions(), new SimpleIteratorEnvironment());
+
+ SortedKeyValueIterator<Key,Value> copy = skvi.deepCopy(new SimpleIteratorEnvironment());
--- End diff --
Further along that note, many user iterators leave deepCopy() as `throw new UnsupportedOperationException()`. They would fail many of these IteratorTestCases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/50#issuecomment-163715371
> but does anyone have thoughts on a path to "graduate" into public API status?
Well currently iterators are not declared as public API.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r47451909
--- Diff: iterator-test-harness/src/main/java/org/apache/accumulo/iteratortest/IteratorTestUtil.java ---
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.iteratortest;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.iterators.SortedMapIterator;
+import org.apache.accumulo.core.iterators.system.ColumnFamilySkippingIterator;
+import org.apache.accumulo.iteratortest.testcases.IteratorTestCase;
+
+/**
+ * A collection of methods that are helpful to the development of {@link IteratorTestCase}s.
+ */
+public class IteratorTestUtil {
+
+ public static SortedKeyValueIterator<Key,Value> instantiateIterator(IteratorTestInput input) {
+ try {
+ return Objects.requireNonNull(input.getIteratorClass()).newInstance();
+ } catch (InstantiationException | IllegalAccessException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ public static SortedKeyValueIterator<Key,Value> createSource(IteratorTestInput input) {
+ return new ColumnFamilySkippingIterator(new SortedMapIterator(Objects.requireNonNull(input).getInput()));
--- End diff --
Turns out, this was a really good suggestion. Helped me find a bug in my test cases (need to make sure to copy the Key-Value being emitted). The Tablet Scanner does this when converting the Key and Value from the iterator stack into a `KeyValue`. Thanks again, Bill!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-626 Iterator fuzzer/tester
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/50#discussion_r44291578
--- Diff: iterator-test-harness/pom.xml ---
@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-project</artifactId>
+ <version>1.8.0-SNAPSHOT</version>
+ </parent>
+ <artifactId>accumulo-iterator-test-harness</artifactId>
+ <name>Apache Accumulo Iterator Test Harness</name>
+ <description>A library for testing Apache Accumulo Iterators.</description>
+ <dependencies>
+ <!-- TODO Don't force downstream users to have JUnit -->
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <!--optional>true</optional-->
+ </dependency>
+ <dependency>
+ <groupId>log4j</groupId>
+ <artifactId>log4j</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.accumulo</groupId>
+ <artifactId>accumulo-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-client</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.reflections</groupId>
+ <artifactId>reflections</artifactId>
+ </dependency>
--- End diff --
Do we need to add this to the NOTICE?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---