You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by denine99 <gi...@git.apache.org> on 2015/03/12 05:40:41 UTC
[GitHub] accumulo pull request: ACCUMULO-3645 Run iterators at major compac...
GitHub user denine99 opened a pull request:
https://github.com/apache/accumulo/pull/22
ACCUMULO-3645 Run iterators at major compaction on empty tablets
Putting this out for review to see if I am on the right track; I don't expect it to be perfect on the first shot. I'd like to redo this for 1.6.3 if it looks good. Open to suggestions on design and on testing compacting empty tablets.
Removed/modified lines in Tablet that stop construction of the SKVI stack when a tablet has no data.
Includes an iterator that generates data, for testing purposes. Includes one integration test. Not sure about MiniAccumulo.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/denine99/accumulo ACCUMULO-3645
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/22.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 #22
----
commit d459b91dbcf02f4057e2a5625a60fb70920ca202
Author: Dylan Hutchison <dh...@mit.edu>
Date: 2015-03-12T04:02:08Z
ACCUMULO-3645 Run iterators not run at major compaction when tablets are empty
Removed/modified lines in Tablet that stop construction of the SKVI stack when a tablet has no data.
Includes an iterator that generates data, for testing purposes. Includes one integration test.
----
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r27338495
--- Diff: test/src/test/java/org/apache/accumulo/test/TableOperationsIT.java ---
@@ -229,4 +239,122 @@ public void createMergeClonedTable() throws Exception {
return map;
}
+ @Test
--- End diff --
Thanks =D
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26355215
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1861,8 +1861,10 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
if (inputFiles.isEmpty()) {
if (reason == MajorCompactionReason.USER) {
// no work to do
- lastCompactID = compactionId.getFirst();
- updateCompactionID = true;
+// lastCompactID = compactionId.getFirst();
--- End diff --
Yes, touching `lastCompactID` is sensitive because Accumulo uses that to determine when a compaction has already occurred. I think the modifications are correct, but I retained the commented lines because I feel unsure. I will remove the commented lines and try to test further.
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-78752724
Anyone have additional test cases to suggest? I covered
* multi-tablet full major compactions,
* multi-tablet non-full major compactions,
* canceled major compactions
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-79018129
Not sure why the asfbot Jenkins CI thingy found a problem. The build, unit tests and the integration tests I wrote work for me, both on MiniAccumulo and standalone.
---
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-3645 Run iterators at major compac...
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/22#discussion_r27324260
--- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/HardListIterator.java ---
@@ -0,0 +1,111 @@
+/*
+ * 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.core.iterators.user;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.PartialKey;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorEnvironment;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.util.PeekingIterator;
+import org.apache.hadoop.io.Text;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+
+/**
+ * A wrapper making a list of hardcoded data into a SKVI. For testing.
+ */
+public class HardListIterator implements SortedKeyValueIterator<Key,Value> {
+ private static final Logger log = Logger.getLogger(HardListIterator.class);
+ public final static SortedMap<Key,Value> allEntriesToInject;
+ static {
+ SortedMap<Key,Value> t = new TreeMap<>();
+ t.put(new Key(new Text("a1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("c1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("m1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ allEntriesToInject = Collections.unmodifiableSortedMap(t); // for safety
+ }
+
+ private PeekingIterator<Map.Entry<Key,Value>> inner;
+ private Range seekRng;
+
+ @Override
+ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env) throws IOException {
--- End diff --
Curious what `env.isFullMajorCompaction()` returns in the case where the tablet has no files. Should probably return true. Could have a test for this
---
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-3645 Run iterators at major compac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26357025
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/EverythingCompactionStrategy.java ---
@@ -27,7 +27,7 @@
@Override
public boolean shouldCompact(MajorCompactionRequest request) throws IOException {
- return request.getFiles().size() > 0;
+ return true; // ACCUMULO-3645 compact for empty files too
--- End diff --
A point to consider: unless it's definitely a bug fix, and we're sure we want to change the behavior to fix that bug, our versioning rules would disallow new functionality to be added in 1.6.3. It'd have to go in 1.7.
---
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-3645 Run iterators at major compac...
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/22#discussion_r27345081
--- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/HardListIterator.java ---
@@ -0,0 +1,111 @@
+/*
+ * 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.core.iterators.user;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.PartialKey;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorEnvironment;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.util.PeekingIterator;
+import org.apache.hadoop.io.Text;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+
+/**
+ * A wrapper making a list of hardcoded data into a SKVI. For testing.
+ */
+public class HardListIterator implements SortedKeyValueIterator<Key,Value> {
+ private static final Logger log = Logger.getLogger(HardListIterator.class);
+ public final static SortedMap<Key,Value> allEntriesToInject;
+ static {
+ SortedMap<Key,Value> t = new TreeMap<>();
+ t.put(new Key(new Text("a1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("c1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("m1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ allEntriesToInject = Collections.unmodifiableSortedMap(t); // for safety
+ }
+
+ private PeekingIterator<Map.Entry<Key,Value>> inner;
+ private Range seekRng;
+
+ @Override
+ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env) throws IOException {
--- End diff --
Your comment made me realize HardListIterator was in core/src/main/java,
should probably be in core/src/test/java.
On Mar 27, 2015 7:33 PM, "Dylan Hutchison" <no...@github.com> wrote:
> In
> core/src/main/java/org/apache/accumulo/core/iterators/user/HardListIterator.java
> <https://github.com/apache/accumulo/pull/22#discussion_r27338487>:
>
> > +public class HardListIterator implements SortedKeyValueIterator<Key,Value> {
> > + private static final Logger log = Logger.getLogger(HardListIterator.class);
> > + public final static SortedMap<Key,Value> allEntriesToInject;
> > + static {
> > + SortedMap<Key,Value> t = new TreeMap<>();
> > + t.put(new Key(new Text("a1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
> > + t.put(new Key(new Text("c1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
> > + t.put(new Key(new Text("m1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
> > + allEntriesToInject = Collections.unmodifiableSortedMap(t); // for safety
> > + }
> > +
> > + private PeekingIterator<Map.Entry<Key,Value>> inner;
> > + private Range seekRng;
> > +
> > + @Override
> > + public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env) throws IOException {
>
> I confirmed that env.isFullMajorCompaction() is true when the tablet has
> no files on a standalone single-node instance.
>
> Writing a test for it is possible by writing an iterator, similar to
> HardListIterator, that emits an entry with a boolean value-- the result of
> env.isFullMajorCompaction(). It does not seem worth it to include a new
> iterator class in the source code for one test like this.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/accumulo/pull/22/files#r27338487>.
>
---
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-3645 Run iterators at major compac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r27348484
--- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/HardListIterator.java ---
@@ -0,0 +1,111 @@
+/*
+ * 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.core.iterators.user;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.PartialKey;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorEnvironment;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.util.PeekingIterator;
+import org.apache.hadoop.io.Text;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+
+/**
+ * A wrapper making a list of hardcoded data into a SKVI. For testing.
+ */
+public class HardListIterator implements SortedKeyValueIterator<Key,Value> {
+ private static final Logger log = Logger.getLogger(HardListIterator.class);
+ public final static SortedMap<Key,Value> allEntriesToInject;
+ static {
+ SortedMap<Key,Value> t = new TreeMap<>();
+ t.put(new Key(new Text("a1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("c1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("m1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ allEntriesToInject = Collections.unmodifiableSortedMap(t); // for safety
+ }
+
+ private PeekingIterator<Map.Entry<Key,Value>> inner;
+ private Range seekRng;
+
+ @Override
+ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env) throws IOException {
--- End diff --
Yes, putting HardListIterator in src/test/java will place it in the test jar instead of the "regular" jar. I don't think we put the core test-jar on the classpath for ITs. You can (should, IMO) put it in test/src/test/java instead of core/src/main/java and that will work for your integration test.
---
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-3645 Run iterators at major compac...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-92995877
If we do not squish down to one commit, we would need to add issue number to all of the commit messages.
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r28030298
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1860,9 +1860,14 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
if (inputFiles.isEmpty()) {
if (reason == MajorCompactionReason.USER) {
- // no work to do
- lastCompactID = compactionId.getFirst();
- updateCompactionID = true;
+ if (compactionId.getSecond().getIterators().isEmpty()) {
+ log.debug("No-op major compaction by USER on 0 input files because no iterators present.");
+ lastCompactID = compactionId.getFirst();
+ updateCompactionID = true;
+ } else {
+ log.debug("Major compaction by USER on 0 input files with iterators.");
+ filesToCompact = new HashMap<FileRef,DataFileValue>();
--- End diff --
We're at Java 7 then? Sounds good.
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r27337627
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -2566,12 +2579,7 @@ public void compactAll(long compactionId, UserCompactionConfig compactionConfig)
if (isClosing() || isClosed() || majorCompactionQueued.contains(MajorCompactionReason.USER) || isMajorCompactionRunning())
return;
- if (getDatafileManager().getDatafileSizes().size() == 0) {
- // no files, so jsut update the metadata table
- majorCompactionState = CompactionState.IN_PROGRESS;
- updateMetadata = true;
- lastCompactID = compactionId;
- } else {
+ {
--- End diff --
No reason, except to make a smaller diff. Removing...
---
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-3645 Run iterators at major compac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26323276
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/EverythingCompactionStrategy.java ---
@@ -27,7 +27,7 @@
@Override
public boolean shouldCompact(MajorCompactionRequest request) throws IOException {
- return request.getFiles().size() > 0;
+ return true; // ACCUMULO-3645 compact for empty files too
--- End diff --
Rather than change the default compaction strategy, it'd probably be better to introduce a new compaction strategy, which returns true, instead of implementing the optimized behavior.
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-84524662
I fixed up the patch from Keith's comments. This one looks good.
My mistake starting out with the patch was trying to change *every* location where an empty tablet could be disregarded during compaction. Now the current patch has the *minimal* changes necessary to make empty tablets compact. The biggest one is the change to the default compaction strategy. As before, I'm open for ideas to test further. I think we're closing in on good code.
To answer the question I posed previously, the expected behavior when a major compaction iterator throws and exception (e.g. [BadIterator](https://accumulo.apache.org/1.6/apidocs/org/apache/accumulo/test/functional/BadIterator.html))
is that the tablet server will fail the compaction, not updating the metadata table, and then retry the compaction. If the error is permanent, then the compaction will fail again and again continuously. The only way to stop the cycle is to cancel the compaction manually. This patch maintains this behavior.
Rebased the commits to master. Will squash once given a green light.
---
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-3645 Run iterators at major compac...
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/22#discussion_r26576497
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -2022,6 +2029,16 @@ protected AccumuloConfiguration createTableConfiguration(TableConfiguration base
private Set<FileRef> removeSmallest(Map<FileRef,DataFileValue> filesToCompact, int maxFilesToCompact) {
// ensure this method works properly when multiple files have the same size
+ // short-circuit; also handles zero files case
+ if (filesToCompact.size() <= maxFilesToCompact) {
+ Set<FileRef> smallestFiles = new HashSet<FileRef>(filesToCompact.size());
--- End diff --
could do
```java
Set<FileRef> smallestFiles = new HashSet<FileRef>(filesToCompact.keySet());
filesToCompact.clear();
return smallestFiles;
```
slightly easier to read than for loop
---
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-3645 Run iterators at major compac...
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/22#discussion_r26574930
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -2567,11 +2585,13 @@ public void compactAll(long compactionId, UserCompactionConfig compactionConfig)
return;
if (getDatafileManager().getDatafileSizes().size() == 0) {
- // no files, so jsut update the metadata table
- majorCompactionState = CompactionState.IN_PROGRESS;
+ // ACCUMULO-3645 do create the iterator stack
+// majorCompactionState = CompactionState.IN_PROGRESS;
updateMetadata = true;
lastCompactID = compactionId;
- } else {
+ }
+// else
--- End diff --
Should not update compaction id in metadata table and initiate compaction
---
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-3645 Run iterators at major compac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26356908
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/EverythingCompactionStrategy.java ---
@@ -27,7 +27,7 @@
@Override
public boolean shouldCompact(MajorCompactionRequest request) throws IOException {
- return request.getFiles().size() > 0;
+ return true; // ACCUMULO-3645 compact for empty files too
--- End diff --
I think I'd only be in favor of making the change for 1.6.3 if it's done as a new CompactionStrategy. I'd need convincing to change the default implementation in a bug fix release without seriously enumerating the corner cases.
---
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-3645 Run iterators at major compac...
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/22#discussion_r27334373
--- Diff: test/src/test/java/org/apache/accumulo/test/TableOperationsIT.java ---
@@ -229,4 +239,122 @@ public void createMergeClonedTable() throws Exception {
return map;
}
+ @Test
--- End diff --
Nice tests!!!!!!!!!!
---
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-3645 Run iterators at major compac...
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/22#discussion_r28244345
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/EverythingCompactionStrategy.java ---
@@ -27,7 +27,7 @@
@Override
public boolean shouldCompact(MajorCompactionRequest request) throws IOException {
--- End diff --
`MajorCompactionRequest` could be modified to add a `getCompactionIterators()` method, then this would have all of the info needed to make a decision. This might clean the code up a little bit in other places. This is just something I thought of while looking at this, not something I think has to be done.
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r27348213
--- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/HardListIterator.java ---
@@ -0,0 +1,111 @@
+/*
+ * 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.core.iterators.user;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.PartialKey;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorEnvironment;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.util.PeekingIterator;
+import org.apache.hadoop.io.Text;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+
+/**
+ * A wrapper making a list of hardcoded data into a SKVI. For testing.
+ */
+public class HardListIterator implements SortedKeyValueIterator<Key,Value> {
+ private static final Logger log = Logger.getLogger(HardListIterator.class);
+ public final static SortedMap<Key,Value> allEntriesToInject;
+ static {
+ SortedMap<Key,Value> t = new TreeMap<>();
+ t.put(new Key(new Text("a1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("c1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("m1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ allEntriesToInject = Collections.unmodifiableSortedMap(t); // for safety
+ }
+
+ private PeekingIterator<Map.Entry<Key,Value>> inner;
+ private Range seekRng;
+
+ @Override
+ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env) throws IOException {
--- End diff --
I tried putting HardListIterator in core/src/test/java when I first wrote it, but then it does not compile (error: cannot find symbol HardListIterator). I think it needs to be on the main compile path to run in a live instance of Accumulo, not the testCompile path.
---
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-3645 Run iterators at major compac...
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/22#discussion_r26574837
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -2548,7 +2565,8 @@ public void compactAll(long compactionId, UserCompactionConfig compactionConfig)
boolean updateMetadata = false;
synchronized (this) {
- if (lastCompactID >= compactionId)
+ // ACCUMULO-3645 compaction may generate results if there is a compaction-scope iterator
+ if (lastCompactID >= compactionId && compactionConfig.getIterators().isEmpty())
--- End diff --
If lastCompactID >= compactionId then should not do anything. When master receives compaction request it increments counter in zookeeper. When a tablet compacts, it records the compaction id in metadata. Master keeps asking tablets to compact until all tablets are counter are >= the zookeeper value. This change could lead to tablet compacting multiple times for a single user compaction request with iters.
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-82421590
Thanks @keith-turner. I will revisit the compaction IDs and get back with another commit.
Do you know what the expected behavior is when we use a BadIterator in a user compaction?
---
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-3645 Run iterators at major compac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-93047530
@keith-turner you can also manually set the author on the commit if you just want to do it yourself via the `--author=<author>` option on `git-commit` (`git checkout`, `git rebase -i`, and `git commit --amend --author=...`). I've done this for a few patches, didn't know if you knew about it.
---
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-3645 Run iterators at major compac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r28030423
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1860,9 +1860,14 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
if (inputFiles.isEmpty()) {
if (reason == MajorCompactionReason.USER) {
- // no work to do
- lastCompactID = compactionId.getFirst();
- updateCompactionID = true;
+ if (compactionId.getSecond().getIterators().isEmpty()) {
+ log.debug("No-op major compaction by USER on 0 input files because no iterators present.");
+ lastCompactID = compactionId.getFirst();
+ updateCompactionID = true;
+ } else {
+ log.debug("Major compaction by USER on 0 input files with iterators.");
+ filesToCompact = new HashMap<FileRef,DataFileValue>();
--- End diff --
Yup, it looks like we moved this fixVersion to 1.7.0 which is on Java7. Accumulo 1.6 and earlier is still on java6
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26355050
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/EverythingCompactionStrategy.java ---
@@ -27,7 +27,7 @@
@Override
public boolean shouldCompact(MajorCompactionRequest request) throws IOException {
- return request.getFiles().size() > 0;
+ return true; // ACCUMULO-3645 compact for empty files too
--- End diff --
The only time this will make a difference is when the USER manually issues a compaction with at least one iterator, in which case the user should expect the compaction occurs by default.
I will add a condition to the Tablet code to keep the "optimized" behavior for empty tablets on user-issued compactions that do not have any iterators configured.
---
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-3645 Run iterators at major compac...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/22
---
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-3645 Run iterators at major compac...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-93048725
I was aware of how to change the author, I just wanted to make sure @denine99 was ok w/ me squishing the commits into one commit
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-93057682
@keith-turner,
Since this is my first 'real' pull request and I could use some practice, I went ahead and squashed into one commit and rebased at master 85d254e12. How's this look?
---
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-3645 Run iterators at major compac...
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/22#discussion_r26576646
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1913,20 +1918,22 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
// compaction was canceled
return majCStats;
}
+ compactionIterators = compactionId.getSecond().getIterators();
synchronized (this) {
- if (lastCompactID >= compactionId.getFirst())
+ // ACCUMULO-3645 run if there are compaction iterators that may generate entries
+ if (lastCompactID >= compactionId.getFirst() && compactionIterators.isEmpty())
--- End diff --
if the user req compaction is already done then should not continue
---
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-3645 Run iterators at major compac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-79086393
Checkstyle violation: src/test/java/org/apache/accumulo/test/TableOperationsIT.java[299] (regexp) RegexpSinglelineJava: Line has trailing whitespace.
https://builds.apache.org/job/Accumulo-Pull-Requests/13/console
If you're only running `mvn package`, checkstyle (and findbugs) won't be invoked. You can either run them manually, or do a `mvn verify -DskipITs` (assuming you otherwise want it to function as `mvn package` does).
---
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-3645 Run iterators at major compac...
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/22#discussion_r27323786
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -2566,12 +2579,7 @@ public void compactAll(long compactionId, UserCompactionConfig compactionConfig)
if (isClosing() || isClosed() || majorCompactionQueued.contains(MajorCompactionReason.USER) || isMajorCompactionRunning())
return;
- if (getDatafileManager().getDatafileSizes().size() == 0) {
- // no files, so jsut update the metadata table
- majorCompactionState = CompactionState.IN_PROGRESS;
- updateMetadata = true;
- lastCompactID = compactionId;
- } else {
+ {
--- End diff --
Why leave this undconditional code block? Did a var name conflict?
---
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-3645 Run iterators at major compac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26323700
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1861,8 +1861,10 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
if (inputFiles.isEmpty()) {
if (reason == MajorCompactionReason.USER) {
// no work to do
- lastCompactID = compactionId.getFirst();
- updateCompactionID = true;
+// lastCompactID = compactionId.getFirst();
--- End diff --
What was your intention in leaving these commented out?
I'm not too familiar with how the compactionId is used, but it seems like getting this right is tricky.
---
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-3645 Run iterators at major compac...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-92387956
+1
I think this change looks good.
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r27338487
--- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/HardListIterator.java ---
@@ -0,0 +1,111 @@
+/*
+ * 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.core.iterators.user;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.PartialKey;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorEnvironment;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.util.PeekingIterator;
+import org.apache.hadoop.io.Text;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+
+/**
+ * A wrapper making a list of hardcoded data into a SKVI. For testing.
+ */
+public class HardListIterator implements SortedKeyValueIterator<Key,Value> {
+ private static final Logger log = Logger.getLogger(HardListIterator.class);
+ public final static SortedMap<Key,Value> allEntriesToInject;
+ static {
+ SortedMap<Key,Value> t = new TreeMap<>();
+ t.put(new Key(new Text("a1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("c1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("m1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ allEntriesToInject = Collections.unmodifiableSortedMap(t); // for safety
+ }
+
+ private PeekingIterator<Map.Entry<Key,Value>> inner;
+ private Range seekRng;
+
+ @Override
+ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env) throws IOException {
--- End diff --
I confirmed that env.isFullMajorCompaction() is true when the tablet has no files on a standalone single-node instance.
Writing a test for it is possible by writing an iterator, similar to HardListIterator, that emits an entry with a boolean value-- the result of env.isFullMajorCompaction(). It does not seem worth it to include a new iterator class in the source code for one test like this.
---
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-3645 Run iterators at major compac...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the pull request:
https://github.com/apache/accumulo/pull/22#issuecomment-92995186
@denine99 I can merge this. Would you mind if I squished it down to one commit (I would maintain you as the author in git)?
---
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-3645 Run iterators at major compac...
Posted by denine99 <gi...@git.apache.org>.
Github user denine99 commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r27715976
--- Diff: core/src/main/java/org/apache/accumulo/core/iterators/user/HardListIterator.java ---
@@ -0,0 +1,111 @@
+/*
+ * 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.core.iterators.user;
+
+import org.apache.accumulo.core.data.ByteSequence;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.PartialKey;
+import org.apache.accumulo.core.data.Range;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.IteratorEnvironment;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+import org.apache.accumulo.core.util.PeekingIterator;
+import org.apache.hadoop.io.Text;
+import org.apache.log4j.Logger;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+import java.util.SortedMap;
+import java.util.TreeMap;
+
+/**
+ * A wrapper making a list of hardcoded data into a SKVI. For testing.
+ */
+public class HardListIterator implements SortedKeyValueIterator<Key,Value> {
+ private static final Logger log = Logger.getLogger(HardListIterator.class);
+ public final static SortedMap<Key,Value> allEntriesToInject;
+ static {
+ SortedMap<Key,Value> t = new TreeMap<>();
+ t.put(new Key(new Text("a1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("c1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ t.put(new Key(new Text("m1"), new Text("colF3"), new Text("colQ3"), System.currentTimeMillis()), new Value("1".getBytes()));
+ allEntriesToInject = Collections.unmodifiableSortedMap(t); // for safety
+ }
+
+ private PeekingIterator<Map.Entry<Key,Value>> inner;
+ private Range seekRng;
+
+ @Override
+ public void init(SortedKeyValueIterator<Key,Value> source, Map<String,String> options, IteratorEnvironment env) throws IOException {
--- End diff --
Forgot to comment on this after committing-- I moved the class into the main code of the test project. I did this so that I could deploy the test code JAR in a standalone Accumulo cluster after calling `mvn package -DskipITs` and then do the test. Maybe the Accumulo build has an automated way to deploy JARs containing test src code in standalone instances via the configuration? If not, this is a good compromise.
---
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-3645 Run iterators at major compac...
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/22#discussion_r26384856
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1861,8 +1861,10 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
if (inputFiles.isEmpty()) {
if (reason == MajorCompactionReason.USER) {
// no work to do
- lastCompactID = compactionId.getFirst();
- updateCompactionID = true;
+// lastCompactID = compactionId.getFirst();
--- End diff --
I will look at this PR Mon.
---
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-3645 Run iterators at major compac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26357063
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1861,8 +1861,10 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
if (inputFiles.isEmpty()) {
if (reason == MajorCompactionReason.USER) {
// no work to do
- lastCompactID = compactionId.getFirst();
- updateCompactionID = true;
+// lastCompactID = compactionId.getFirst();
--- End diff --
I'd definitely feel more comfortable if @keith-turner reviewed it. He's the last person I know who fixed a bug in this code.
---
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-3645 Run iterators at major compac...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r28030148
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java ---
@@ -1860,9 +1860,14 @@ private CompactionStats _majorCompact(MajorCompactionReason reason) throws IOExc
if (inputFiles.isEmpty()) {
if (reason == MajorCompactionReason.USER) {
- // no work to do
- lastCompactID = compactionId.getFirst();
- updateCompactionID = true;
+ if (compactionId.getSecond().getIterators().isEmpty()) {
+ log.debug("No-op major compaction by USER on 0 input files because no iterators present.");
+ lastCompactID = compactionId.getFirst();
+ updateCompactionID = true;
+ } else {
+ log.debug("Major compaction by USER on 0 input files with iterators.");
+ filesToCompact = new HashMap<FileRef,DataFileValue>();
--- End diff --
nit, you can just use the diamond operator
---
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-3645 Run iterators at major compac...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/22#discussion_r26323346
--- Diff: core/src/main/java/org/apache/accumulo/core/iterators/HardListIterator.java ---
@@ -0,0 +1,109 @@
+/*
+ * 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.core.iterators;
--- End diff --
This should be in the "users" sub-package.
---
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.
---