accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ACCUMULO-2817) Add offset and limit arguments to byte array Encoder.decode method
Date Mon, 23 Feb 2015 19:19:11 GMT

    [ https://issues.apache.org/jira/browse/ACCUMULO-2817?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14333683#comment-14333683
] 

Josh Elser commented on ACCUMULO-2817:
--------------------------------------

{code}
diff --git a/core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java
b/core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java
index 030f4ec..72f706a 100644
--- a/core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java
+++ b/core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java
@@ -449,6 +449,17 @@ package org.apache.accumulo.core.replication.proto;
         throws com.google.protobuf.InvalidProtocolBufferException {
       return PARSER.parseFrom(data, extensionRegistry);
     }
+    public static org.apache.accumulo.core.replication.proto.Replication.Status parseFrom(
+            byte[] data, int offset, int len)
+            throws com.google.protobuf.InvalidProtocolBufferException {
+      return PARSER.parseFrom(data, offset, len);
+    }
+    public static org.apache.accumulo.core.replication.proto.Replication.Status parseFrom(
+            byte[] data, int offset, int len,
+            com.google.protobuf.ExtensionRegistryLite extensionRegistry)
+            throws com.google.protobuf.InvalidProtocolBufferException {
+      return PARSER.parseFrom(data, offset, len, extensionRegistry);
+    }
     public static org.apache.accumulo.core.replication.proto.Replication.Status parseFrom(java.io.InputStream
input)
         throws java.io.IOException {
       return PARSER.parseFrom(input);
{code}

This is generated code. You don't need to be changing it. {{StatusCombiner}} will just have
to be changed to call the necessary method on {{Status}} if no methods are exposed already.

{code}
    try {
      Preconditions.checkNotNull(b, "cannot decode null byte array");
    } catch (NullPointerException e) {
      throw new ValueFormatException(e);
    }

    try {
      Preconditions.checkArgument(offset >= 0, "offset %s cannot be negative",
              offset);
    } catch (IllegalArgumentException e) {
      throw new ValueFormatException(e);
    }

    try {
      Preconditions.checkArgument(offset + len < b.length, "offset + length %s exceeds
byte array length %s",
              (offset + len), b.length);
    } catch (IllegalArgumentException e) {
      throw new ValueFormatException(e);
    }
{code}

You don't need to catch the Exceptions thrown by Preconditions. They're RuntimeExceptions
-- just let them filter up.

bq. Do I add "@since" to any public methods, or only ones implementing an Interface, or all
new methods?

Adding it to AbstractEncoder as you did is good. I think AbstractLexicoder is better off being
in {{org.apache.accumulo.core.client.lexicoder.impl}} which would not need to have the annotation.
Generally speaking, if you're introducing a new class into the [public API|https://github.com/apache/accumulo/blob/master/README.md#api],
you can just add it at the class level and you don't need the method-level annotations. When
new methods are added into that class later, {{since}} would be added on those methods.

{code}
  /**
   * Calls {@link #decodeUnchecked(byte[], int, int)} as {@code decodeUnchecked(b, 0, b.length);}.
   * Does not check if b is null.
   */
  @Override
  public T decode(byte[] b) {
    return decodeUnchecked(b, 0, b.length);
  }
{code}

Why not check that {{b}} is non-null?

bq. I looked through semver, and I don't think "any backwards incompatible changes are introduced
to the public API," but feel free to correct me.

The concern is that a user who implemented their own Lexicoder against 1.6.x would suddenly
stop working with 1.7.0. The way you currently did it looks fine to me (Lexicoder hasn't ultimately
changed only the implementations in such a way that only adds a new method).

> Add offset and limit arguments to byte array Encoder.decode method
> ------------------------------------------------------------------
>
>                 Key: ACCUMULO-2817
>                 URL: https://issues.apache.org/jira/browse/ACCUMULO-2817
>             Project: Accumulo
>          Issue Type: Improvement
>          Components: client
>            Reporter: Josh Elser
>            Assignee: Matt Dailey
>              Labels: newbie
>             Fix For: 1.7.0
>
>         Attachments: ACCUMULO-2817.patch
>
>
> Similar to ACCUMULO-2445, but presently the encoder only works on complete byte arrays.
This forces an extra copy of the data when it is located in an array that contains other information
(e.g. a composite key).
> It would be nice to be able to provide offset and length arguments to {{Encoder.decode}}
so that users can avoid the additional arraycopy.
> Changing to a ByteBuffer instead of byte array argument would also be acceptable, but
more churn on the API that, unless it's happening globally, I would rather avoid. It would
also incur the penalty for that extra Object, which while minimal alone, could be significant
if decoding every value in a table, for example.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message