You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2023/04/08 16:10:16 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #3286: Add support for storing ranges in DataFileValue
cshannon opened a new pull request, #3286:
URL: https://github.com/apache/accumulo/pull/3286
This adds support for storing a collection of ranges associated with a data file in the Accumulo metadata table. This will allow the ability to fence off RFiles by ranges in the future so we can accomplish no-chop merges. The previous CSV format of the serialized DataFileValue has been converted to Json and the ranges are Base64 encoded.
This is the second part of #1327 and will provide a mechanism to store ranges. Note that this PR doesn't actually update Accumulo to start storing ranges yet, it just adds support to do so. When this PR and #3246 is ready, a follow on task would be to go back and update the merge code to no longer run chop compactions but to actually start populating ranges in the metadata, etc.
The goal here is to be able to optionally store ranges for a file in metadata. If the range list is empty then that would just mean the entire file is used (infinite range) and this is nice as it allows us to be backwards compatible with previous metadata that didn't include a range and also allows us to not have to store anything unless we need to fence the file off in the future (such as when merging).
One question I had was what the size and numEntries values should be now inside of DataFileValue. Will those values stay as they currently are (size and numEntries for the entire file) or do we need to set those values based on the Ranges provided? Ie do we need to try and reduce the size somehow based on the fenced off values vs the actual values? I don't think so, I think we can leave the values alone (apply to the whole file) and just get what we need out of the ranges as those values are used for things like splits which I would still think you'd want to take into account the entire file for but maybe someone else with more background can comment on that.
A couple of other notes about this PR:
1. I made it a draft PR for now as this should really be target at a 3.1 branch and right now main is just 3.0 plus I wanted to get some initial feedback.
2. DataFileValue format is now a JSON string when serialized into the metadata table instead of a CSV. Reading of the old format is detected by checking for a Json parsing exception and falling back to the legacy format.
3. Ranges are encoded as Base64. The simplest and most compact way to store a range was to encode the entire Range into a byte array and then encode that as Base 64 as ranges are complex with keys and a lot of binary values.
4. I left the equals/hashcode methods alone for now in DataFileValue and they only compare the size and number of entries. I wasn't sure if we wanted to also add the range comparison to the equals yet, it probably depends how we intend to use it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3286: Add support for storing ranges in DataFileValue
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#discussion_r1166222165
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/DataFileValue.java:
##########
@@ -20,42 +20,76 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iteratorsImpl.system.InterruptibleIterator;
import org.apache.accumulo.core.iteratorsImpl.system.TimeSettingIterator;
+import org.apache.accumulo.core.util.json.RangeAdapter;
+
+import com.google.gson.Gson;
+import com.google.gson.JsonSyntaxException;
public class DataFileValue {
- private long size;
- private long numEntries;
+
+ private static final Gson gson = RangeAdapter.createRangeGson();
+
+ private final long size;
+ private final long numEntries;
private long time = -1;
+ private final List<Range> ranges;
- public DataFileValue(long size, long numEntries, long time) {
- this.size = size;
- this.numEntries = numEntries;
- this.time = time;
+ public DataFileValue(final long size, final long numEntries, final long time) {
+ this(size, numEntries, time, null);
+ }
+
+ public DataFileValue(final long size, final long numEntries) {
+ this(size, numEntries, null);
}
- public DataFileValue(long size, long numEntries) {
+ public DataFileValue(final long size, final long numEntries, final long time,
+ final Collection<Range> ranges) {
this.size = size;
this.numEntries = numEntries;
- this.time = -1;
+ this.time = time;
+ // If ranges is null then just set to empty list and also merge overlapping to reduce
+ // the data stored if possible.
+ this.ranges = Optional.ofNullable(ranges).map(Range::mergeOverlapping)
+ .map(Collections::unmodifiableList).orElse(List.of());
}
- public DataFileValue(String encodedDFV) {
- String[] ba = encodedDFV.split(",");
-
- size = Long.parseLong(ba[0]);
- numEntries = Long.parseLong(ba[1]);
+ public DataFileValue(long size, long numEntries, Collection<Range> ranges) {
+ this(size, numEntries, -1, ranges);
+ }
- if (ba.length == 3) {
- time = Long.parseLong(ba[2]);
- } else {
- time = -1;
+ public static DataFileValue decode(String encodedDFV) {
+ try {
+ // Optimistically try and decode from Json and fall back to decoding the legacy format
+ // if json parsing fails. Over time the old format will be replaced with the new format
+ // as new values are written or updated.
+ return gson.fromJson(encodedDFV, DataFileValue.class);
+ } catch (JsonSyntaxException e) {
+ return decodeLegacy(encodedDFV);
Review Comment:
This could be a follow on issue/PR. If we convert all ranges in the upgrade code then we don't have to have the legacy fallback here, this will stick around in the code long after its actually needed. If only have the conversion in the upgrade code it will make this code a bit cleaner. We create Upgrader classes for going between each version and eventually those are dropped along with the legacy code.
This is the metadata upgrade code https://github.com/apache/accumulo/blob/30d823def879234b4f390f7b816cef670d477f88/server/manager/src/main/java/org/apache/accumulo/manager/upgrade/Upgrader10to11.java#L112
But not sure that is code for going from 2.0 to 2.1 or 2.1 to 3.x. Not sure if we have created a new Upgrader class for going from 2.1
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/DataFileValue.java:
##########
@@ -20,42 +20,76 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iteratorsImpl.system.InterruptibleIterator;
import org.apache.accumulo.core.iteratorsImpl.system.TimeSettingIterator;
+import org.apache.accumulo.core.util.json.RangeAdapter;
+
+import com.google.gson.Gson;
+import com.google.gson.JsonSyntaxException;
public class DataFileValue {
- private long size;
- private long numEntries;
+
+ private static final Gson gson = RangeAdapter.createRangeGson();
+
+ private final long size;
+ private final long numEntries;
private long time = -1;
+ private final List<Range> ranges;
- public DataFileValue(long size, long numEntries, long time) {
- this.size = size;
- this.numEntries = numEntries;
- this.time = time;
+ public DataFileValue(final long size, final long numEntries, final long time) {
+ this(size, numEntries, time, null);
+ }
+
+ public DataFileValue(final long size, final long numEntries) {
+ this(size, numEntries, null);
}
- public DataFileValue(long size, long numEntries) {
+ public DataFileValue(final long size, final long numEntries, final long time,
+ final Collection<Range> ranges) {
this.size = size;
this.numEntries = numEntries;
- this.time = -1;
+ this.time = time;
+ // If ranges is null then just set to empty list and also merge overlapping to reduce
+ // the data stored if possible.
+ this.ranges = Optional.ofNullable(ranges).map(Range::mergeOverlapping)
Review Comment:
Accumulo uses null a lot in the code and its not a great practice. We could not accept null in this new code, if there are no ranges just pass in empty list.
##########
core/src/test/java/org/apache/accumulo/core/metadata/schema/DataFileValueTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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
+ *
+ * https://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.metadata.schema;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Range;
+import org.junit.jupiter.api.Test;
+
+public class DataFileValueTest {
+
+ @Test
+ public void testNoTimeOrRanges() {
+ final String json = new DataFileValue(555, 23).encodeAsString();
+ final DataFileValue dfv = DataFileValue.decode(json);
+
+ assertEquals(555, dfv.getSize());
+ assertEquals(23, dfv.getNumEntries());
+ assertEquals(-1, dfv.getTime());
+ assertEquals(0, dfv.getRanges().size());
+ }
+
+ @Test
+ public void testTimeNoRanges() {
+ long time = System.currentTimeMillis();
+ final String json = new DataFileValue(555, 23, time).encodeAsString();
+ final DataFileValue dfv = DataFileValue.decode(json);
+
+ assertEquals(555, dfv.getSize());
+ assertEquals(23, dfv.getNumEntries());
+ assertEquals(time, dfv.getTime());
+ assertEquals(0, dfv.getRanges().size());
+ }
+
+ @Test
+ public void testInfiniteRange() {
+ long time = System.currentTimeMillis();
+ final DataFileValue dfv = new DataFileValue(555, 23, time, List.of(new Range()));
+ assertEquals(1, dfv.getRanges().size());
+ assertEquals(new Range(), dfv.getRanges().get(0));
+
+ // Verify serialization
+ final String json = new DataFileValue(555, 23, time, List.of(new Range())).encodeAsString();
+ final DataFileValue decoded = DataFileValue.decode(json);
+
+ assertEquals(555, decoded.getSize());
+ assertEquals(23, decoded.getNumEntries());
+ assertEquals(time, decoded.getTime());
+ assertEquals(1, decoded.getRanges().size());
+ assertEquals(new Range(), decoded.getRanges().get(0));
+ }
+
+ @Test
+ public void testComplexRange() {
+ final Key key1 = new Key("row1", "cf1", "cq1", 50);
+ final Key key2 = new Key("row2", "cf2", "cq2", 100);
+ final String json = new DataFileValue(555, 23, List.of(new Range(key1, key2))).encodeAsString();
+ final DataFileValue dfv = DataFileValue.decode(json);
+
+ assertEquals(555, dfv.getSize());
+ assertEquals(23, dfv.getNumEntries());
+ assertEquals(-1, dfv.getTime());
+ assertEquals(1, dfv.getRanges().size());
+ assertEquals(new Range(key1, key2), dfv.getRanges().get(0));
+ }
+
+ @Test
+ public void testOverlappingRanges() {
+ long time = System.currentTimeMillis();
+ final DataFileValue dfv =
+ new DataFileValue(555, 23, time, List.of(new Range("abc", "xyz"), new Range("def")));
+ // Second range is part of first range so should collapse into 1
+ assertEquals(1, dfv.getRanges().size());
+ assertEquals(new Range("abc", "xyz"), dfv.getRanges().get(0));
+ }
+
+ @Test
+ public void testMultipleRanges() {
+ long time = System.currentTimeMillis();
+ final DataFileValue dfv =
+ new DataFileValue(555, 23, time, List.of(new Range("abc", "xyz"), new Range("123", "789")));
+
+ // Ranges will be sorted
+ assertEquals(2, dfv.getRanges().size());
+ assertEquals(new Range("123", "789"), dfv.getRanges().get(0));
+ assertEquals(new Range("abc", "xyz"), dfv.getRanges().get(1));
Review Comment:
Metadata data ranges are exclusive,inclusive. Like [this code](https://github.com/apache/accumulo/blob/30d823def879234b4f390f7b816cef670d477f88/core/src/main/java/org/apache/accumulo/core/dataImpl/KeyExtent.java#L372) converts creates a data range for a tablet. So should test this case, could do the following.
```suggestion
new DataFileValue(555, 23, time, List.of(new Range("abc", false, "xyz", true), new Range("123", "789")));
// Ranges will be sorted
assertEquals(2, dfv.getRanges().size());
assertEquals(new Range("123", "789"), dfv.getRanges().get(0));
assertEquals(new Range("abc",false, "xyz",true), dfv.getRanges().get(1));
```
##########
core/src/test/java/org/apache/accumulo/core/metadata/schema/DataFileValueTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * 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
+ *
+ * https://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.metadata.schema;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.List;
+
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.Range;
+import org.junit.jupiter.api.Test;
+
+public class DataFileValueTest {
+
+ @Test
+ public void testNoTimeOrRanges() {
+ final String json = new DataFileValue(555, 23).encodeAsString();
+ final DataFileValue dfv = DataFileValue.decode(json);
+
+ assertEquals(555, dfv.getSize());
+ assertEquals(23, dfv.getNumEntries());
+ assertEquals(-1, dfv.getTime());
+ assertEquals(0, dfv.getRanges().size());
+ }
+
+ @Test
+ public void testTimeNoRanges() {
+ long time = System.currentTimeMillis();
+ final String json = new DataFileValue(555, 23, time).encodeAsString();
+ final DataFileValue dfv = DataFileValue.decode(json);
+
+ assertEquals(555, dfv.getSize());
+ assertEquals(23, dfv.getNumEntries());
+ assertEquals(time, dfv.getTime());
+ assertEquals(0, dfv.getRanges().size());
+ }
+
+ @Test
+ public void testInfiniteRange() {
+ long time = System.currentTimeMillis();
+ final DataFileValue dfv = new DataFileValue(555, 23, time, List.of(new Range()));
+ assertEquals(1, dfv.getRanges().size());
+ assertEquals(new Range(), dfv.getRanges().get(0));
+
+ // Verify serialization
+ final String json = new DataFileValue(555, 23, time, List.of(new Range())).encodeAsString();
+ final DataFileValue decoded = DataFileValue.decode(json);
+
+ assertEquals(555, decoded.getSize());
+ assertEquals(23, decoded.getNumEntries());
+ assertEquals(time, decoded.getTime());
+ assertEquals(1, decoded.getRanges().size());
+ assertEquals(new Range(), decoded.getRanges().get(0));
+ }
+
+ @Test
+ public void testComplexRange() {
+ final Key key1 = new Key("row1", "cf1", "cq1", 50);
+ final Key key2 = new Key("row2", "cf2", "cq2", 100);
+ final String json = new DataFileValue(555, 23, List.of(new Range(key1, key2))).encodeAsString();
+ final DataFileValue dfv = DataFileValue.decode(json);
+
+ assertEquals(555, dfv.getSize());
+ assertEquals(23, dfv.getNumEntries());
+ assertEquals(-1, dfv.getTime());
+ assertEquals(1, dfv.getRanges().size());
+ assertEquals(new Range(key1, key2), dfv.getRanges().get(0));
+ }
+
+ @Test
+ public void testOverlappingRanges() {
+ long time = System.currentTimeMillis();
+ final DataFileValue dfv =
+ new DataFileValue(555, 23, time, List.of(new Range("abc", "xyz"), new Range("def")));
+ // Second range is part of first range so should collapse into 1
+ assertEquals(1, dfv.getRanges().size());
+ assertEquals(new Range("abc", "xyz"), dfv.getRanges().get(0));
+ }
+
+ @Test
+ public void testMultipleRanges() {
+ long time = System.currentTimeMillis();
+ final DataFileValue dfv =
+ new DataFileValue(555, 23, time, List.of(new Range("abc", "xyz"), new Range("123", "789")));
+
+ // Ranges will be sorted
+ assertEquals(2, dfv.getRanges().size());
+ assertEquals(new Range("123", "789"), dfv.getRanges().get(0));
+ assertEquals(new Range("abc", "xyz"), dfv.getRanges().get(1));
+
+ // Test encoding
+ final String json =
+ new DataFileValue(700, 25, time, List.of(new Range("abc", "xyz"), new Range("123", "789")))
+ .encodeAsString();
+ final DataFileValue decoded = DataFileValue.decode(json);
+
+ assertEquals(700, decoded.getSize());
+ assertEquals(25, decoded.getNumEntries());
+ assertEquals(time, decoded.getTime());
+ assertEquals(2, decoded.getRanges().size());
+ assertEquals(2, decoded.getRanges().size());
Review Comment:
Seems like a duplicate line
```suggestion
assertEquals(2, decoded.getRanges().size());
```
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/DataFileValue.java:
##########
@@ -20,42 +20,76 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iteratorsImpl.system.InterruptibleIterator;
import org.apache.accumulo.core.iteratorsImpl.system.TimeSettingIterator;
+import org.apache.accumulo.core.util.json.RangeAdapter;
+
+import com.google.gson.Gson;
+import com.google.gson.JsonSyntaxException;
public class DataFileValue {
- private long size;
- private long numEntries;
+
+ private static final Gson gson = RangeAdapter.createRangeGson();
+
+ private final long size;
+ private final long numEntries;
private long time = -1;
+ private final List<Range> ranges;
Review Comment:
We need row ranges, but the Range class if for Key ranges so it includes row,col,vis,timestamp, etc. So range include more than we need, wonder how this impact serialization. Wondering it results in extra unnecessary data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3286: Add support for storing ranges in DataFileValue
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#issuecomment-1507842193
> Will those values stay as they currently are (size and numEntries for the entire file) or do we need to set those values based on the Ranges provided?
We can probably just keep a number per file, possibly summing it on merge. If we have the following
* Tablet T1 covering range R1 has file F1 with est size S1
* Tablet T2 covering range R2 has file F1 with est size S2
If we merge T1 and T2 into T3 then T3 could have an estimated size of (S1+S2) for file F1 and ranges R1,R2 for F1. One case I can think of where may want to store S1 and S2 instead of summing them is future splits of a merged tablet. Like if we split T3, how will we estimate its size?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3286: Add support for storing ranges in DataFileValue
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#discussion_r1166682670
##########
core/src/main/java/org/apache/accumulo/core/util/json/RangeAdapter.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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
+ *
+ * https://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.util.json;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.reflect.Type;
+import java.util.Objects;
+
+import org.apache.accumulo.core.data.Range;
+import org.apache.hadoop.io.DataInputBuffer;
+import org.apache.hadoop.io.DataOutputBuffer;
+
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonDeserializationContext;
+import com.google.gson.JsonDeserializer;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonParseException;
+import com.google.gson.JsonSerializationContext;
+import com.google.gson.JsonSerializer;
+
+/**
+ * Gson adapter that can serialize {@link Range} objects to Base64 encoding.
+ *
+ * Ranges will first be serialized to a byte array and then to Base64. Null ranges will be
+ * serialized as a byte array of size 0.
+ */
+public class RangeAdapter implements JsonSerializer<Range>, JsonDeserializer<Range> {
+
+ private static final ByteArrayToBase64TypeAdapter BASE64_SERIALIZER =
+ new ByteArrayToBase64TypeAdapter();
+
+ @Override
+ public Range deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
+ throws JsonParseException {
+ return decodeRange(BASE64_SERIALIZER.deserialize(json, typeOfT, context));
+ }
+
+ @Override
+ public JsonElement serialize(Range src, Type typeOfSrc, JsonSerializationContext context) {
+ return BASE64_SERIALIZER.serialize(encodeRange(src), typeOfSrc, context);
+ }
+
+ /**
+ * Helper methods to encode and decode a range to/from byte arrays. Note that the Gson serializer
+ * won't pass in null values as they are handled separately
+ */
+
+ private static byte[] encodeRange(final Range range) {
+ try {
+ try (DataOutputBuffer buffer = new DataOutputBuffer()) {
+ range.write(buffer);
+ return buffer.getData();
+ }
+ } catch (IOException e) {
Review Comment:
Good catch, I will fix that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3286: Add support for storing ranges in DataFileValue
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#issuecomment-1536433278
Closing this for now as a different approach is being taken to store a range along with the file in the CQ and having multiple entries in metadata for a file (1 per range) vs storing ranges in the value itself
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon closed pull request #3286: Add support for storing ranges in DataFileValue
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon closed pull request #3286: Add support for storing ranges in DataFileValue
URL: https://github.com/apache/accumulo/pull/3286
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3286: Add support for storing ranges in DataFileValue
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#discussion_r1166679798
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/DataFileValue.java:
##########
@@ -20,42 +20,76 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iteratorsImpl.system.InterruptibleIterator;
import org.apache.accumulo.core.iteratorsImpl.system.TimeSettingIterator;
+import org.apache.accumulo.core.util.json.RangeAdapter;
+
+import com.google.gson.Gson;
+import com.google.gson.JsonSyntaxException;
public class DataFileValue {
- private long size;
- private long numEntries;
+
+ private static final Gson gson = RangeAdapter.createRangeGson();
+
+ private final long size;
+ private final long numEntries;
private long time = -1;
+ private final List<Range> ranges;
- public DataFileValue(long size, long numEntries, long time) {
- this.size = size;
- this.numEntries = numEntries;
- this.time = time;
+ public DataFileValue(final long size, final long numEntries, final long time) {
+ this(size, numEntries, time, null);
+ }
+
+ public DataFileValue(final long size, final long numEntries) {
+ this(size, numEntries, null);
}
- public DataFileValue(long size, long numEntries) {
+ public DataFileValue(final long size, final long numEntries, final long time,
+ final Collection<Range> ranges) {
this.size = size;
this.numEntries = numEntries;
- this.time = -1;
+ this.time = time;
+ // If ranges is null then just set to empty list and also merge overlapping to reduce
+ // the data stored if possible.
+ this.ranges = Optional.ofNullable(ranges).map(Range::mergeOverlapping)
+ .map(Collections::unmodifiableList).orElse(List.of());
}
- public DataFileValue(String encodedDFV) {
- String[] ba = encodedDFV.split(",");
-
- size = Long.parseLong(ba[0]);
- numEntries = Long.parseLong(ba[1]);
+ public DataFileValue(long size, long numEntries, Collection<Range> ranges) {
+ this(size, numEntries, -1, ranges);
+ }
- if (ba.length == 3) {
- time = Long.parseLong(ba[2]);
- } else {
- time = -1;
+ public static DataFileValue decode(String encodedDFV) {
+ try {
+ // Optimistically try and decode from Json and fall back to decoding the legacy format
+ // if json parsing fails. Over time the old format will be replaced with the new format
+ // as new values are written or updated.
+ return gson.fromJson(encodedDFV, DataFileValue.class);
+ } catch (JsonSyntaxException e) {
+ return decodeLegacy(encodedDFV);
Review Comment:
I think that is a good point about the code sticking around a long time which is never great. I was thinking about the upgrade option a bit after @ctubbsii mentioned it but I decided against doing the automatic upgrade in this PR for now just because I wanted to get the new format finalized first for the initial draft version. I agree that once this is finalized/merged we can do a follow on issue for upgrades when the format has been fully decided on and then take away the legacy fallback code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3286: Add support for storing ranges in DataFileValue
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#discussion_r1164548163
##########
core/src/main/java/org/apache/accumulo/core/util/json/RangeAdapter.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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
+ *
+ * https://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.util.json;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.reflect.Type;
+import java.util.Objects;
+
+import org.apache.accumulo.core.data.Range;
+import org.apache.hadoop.io.DataInputBuffer;
+import org.apache.hadoop.io.DataOutputBuffer;
+
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonDeserializationContext;
+import com.google.gson.JsonDeserializer;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonParseException;
+import com.google.gson.JsonSerializationContext;
+import com.google.gson.JsonSerializer;
+
+/**
+ * Gson adapter that can serialize {@link Range} objects to Base64 encoding.
+ *
+ * Ranges will first be serialized to a byte array and then to Base64. Null ranges will be
+ * serialized as a byte array of size 0.
+ */
+public class RangeAdapter implements JsonSerializer<Range>, JsonDeserializer<Range> {
+
+ private static final ByteArrayToBase64TypeAdapter BASE64_SERIALIZER =
+ new ByteArrayToBase64TypeAdapter();
+
+ @Override
+ public Range deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
+ throws JsonParseException {
+ return decodeRange(BASE64_SERIALIZER.deserialize(json, typeOfT, context));
+ }
+
+ @Override
+ public JsonElement serialize(Range src, Type typeOfSrc, JsonSerializationContext context) {
+ return BASE64_SERIALIZER.serialize(encodeRange(src), typeOfSrc, context);
+ }
+
+ /**
+ * Helper methods to encode and decode a range to/from byte arrays. Note that the Gson serializer
+ * won't pass in null values as they are handled separately
+ */
+
+ private static byte[] encodeRange(final Range range) {
+ try {
+ try (DataOutputBuffer buffer = new DataOutputBuffer()) {
+ range.write(buffer);
+ return buffer.getData();
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
+
+ private static Range decodeRange(byte[] serialized) {
+ try {
+ try (DataInputBuffer buffer = new DataInputBuffer()) {
+ final Range range = new Range();
+ buffer.reset(serialized, serialized.length);
+ range.readFields(buffer);
+ return range;
+ }
+ } catch (IOException e) {
Review Comment:
```suggestion
try (DataInputBuffer buffer = new DataInputBuffer()) {
final Range range = new Range();
buffer.reset(serialized, serialized.length);
range.readFields(buffer);
return range;
} catch (IOException e) {
```
I think the nested try block is redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #3286: Add support for storing ranges in DataFileValue
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#discussion_r1164547166
##########
core/src/main/java/org/apache/accumulo/core/util/json/RangeAdapter.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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
+ *
+ * https://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.util.json;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.reflect.Type;
+import java.util.Objects;
+
+import org.apache.accumulo.core.data.Range;
+import org.apache.hadoop.io.DataInputBuffer;
+import org.apache.hadoop.io.DataOutputBuffer;
+
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonDeserializationContext;
+import com.google.gson.JsonDeserializer;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonParseException;
+import com.google.gson.JsonSerializationContext;
+import com.google.gson.JsonSerializer;
+
+/**
+ * Gson adapter that can serialize {@link Range} objects to Base64 encoding.
+ *
+ * Ranges will first be serialized to a byte array and then to Base64. Null ranges will be
+ * serialized as a byte array of size 0.
+ */
+public class RangeAdapter implements JsonSerializer<Range>, JsonDeserializer<Range> {
+
+ private static final ByteArrayToBase64TypeAdapter BASE64_SERIALIZER =
+ new ByteArrayToBase64TypeAdapter();
+
+ @Override
+ public Range deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
+ throws JsonParseException {
+ return decodeRange(BASE64_SERIALIZER.deserialize(json, typeOfT, context));
+ }
+
+ @Override
+ public JsonElement serialize(Range src, Type typeOfSrc, JsonSerializationContext context) {
+ return BASE64_SERIALIZER.serialize(encodeRange(src), typeOfSrc, context);
+ }
+
+ /**
+ * Helper methods to encode and decode a range to/from byte arrays. Note that the Gson serializer
+ * won't pass in null values as they are handled separately
+ */
+
+ private static byte[] encodeRange(final Range range) {
+ try {
+ try (DataOutputBuffer buffer = new DataOutputBuffer()) {
+ range.write(buffer);
+ return buffer.getData();
+ }
+ } catch (IOException e) {
Review Comment:
```suggestion
try (DataOutputBuffer buffer = new DataOutputBuffer()) {
range.write(buffer);
return buffer.getData();
} catch (IOException e) {
```
I think the nested try block is redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3286: Add support for storing ranges in DataFileValue
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#discussion_r1166700511
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/DataFileValue.java:
##########
@@ -20,42 +20,76 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iteratorsImpl.system.InterruptibleIterator;
import org.apache.accumulo.core.iteratorsImpl.system.TimeSettingIterator;
+import org.apache.accumulo.core.util.json.RangeAdapter;
+
+import com.google.gson.Gson;
+import com.google.gson.JsonSyntaxException;
public class DataFileValue {
- private long size;
- private long numEntries;
+
+ private static final Gson gson = RangeAdapter.createRangeGson();
+
+ private final long size;
+ private final long numEntries;
private long time = -1;
+ private final List<Range> ranges;
- public DataFileValue(long size, long numEntries, long time) {
- this.size = size;
- this.numEntries = numEntries;
- this.time = time;
+ public DataFileValue(final long size, final long numEntries, final long time) {
+ this(size, numEntries, time, null);
+ }
+
+ public DataFileValue(final long size, final long numEntries) {
+ this(size, numEntries, null);
}
- public DataFileValue(long size, long numEntries) {
+ public DataFileValue(final long size, final long numEntries, final long time,
+ final Collection<Range> ranges) {
this.size = size;
this.numEntries = numEntries;
- this.time = -1;
+ this.time = time;
+ // If ranges is null then just set to empty list and also merge overlapping to reduce
+ // the data stored if possible.
+ this.ranges = Optional.ofNullable(ranges).map(Range::mergeOverlapping)
Review Comment:
I'm fine requiring non-null and making the user just pass an empty list so I can switch it. I wasn't sure which we to go here here as whether or not you allow null and just convert to an empty list or make the user pass in an empty list is mostly preference and I wasn't sure if there was an established pattern people preferred. The other option is to always require at least 1 range (the range could be infinite if the whole file) but that seems pointless and I like the idea of having an empty list just mean the entire file as it's backwards compatible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3286: Add support for storing ranges in DataFileValue
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3286:
URL: https://github.com/apache/accumulo/pull/3286#discussion_r1166712180
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/DataFileValue.java:
##########
@@ -20,42 +20,76 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iteratorsImpl.system.InterruptibleIterator;
import org.apache.accumulo.core.iteratorsImpl.system.TimeSettingIterator;
+import org.apache.accumulo.core.util.json.RangeAdapter;
+
+import com.google.gson.Gson;
+import com.google.gson.JsonSyntaxException;
public class DataFileValue {
- private long size;
- private long numEntries;
+
+ private static final Gson gson = RangeAdapter.createRangeGson();
+
+ private final long size;
+ private final long numEntries;
private long time = -1;
+ private final List<Range> ranges;
Review Comment:
I think we should keep using the Range class itself even if we only care about rows and the other parts of the Key are null/not set as the Range class has a lot of nice utility methods like clip, bound and mergeOverlapping that are going to be quite useful. So it seems like an ideal object to use here.
However, you make a good point about the serialization part of it. Since we only care about the rows and you mentioned in another comment that metadata ranges are exclusive/inclusive then I could do some custom serialization. Right now I'm encoding the entire Range object to binary and then to Base64 but that includes a lot of extra flags and data to support all those fields in a Key that we could leave out if we don't need it. Instead I could probably just update the Range Json adapter I created and only serialize the row portions of the start/end Key (after encoding each byte array to Base 64) as that should cut down on the amount of data that needs to be written and read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org