avro-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From dk...@apache.org
Subject [avro] branch master updated: [AVRO-2051] Remove synchronization for JsonProperties.getJsonProp Fixes #245
Date Wed, 07 Nov 2018 17:05:41 GMT
This is an automated email from the ASF dual-hosted git repository.

dkulp pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/avro.git


The following commit(s) were added to refs/heads/master by this push:
     new a318a34  [AVRO-2051] Remove synchronization for JsonProperties.getJsonProp Fixes
#245
a318a34 is described below

commit a318a34d1f5f8146fb441c092c73b6170719e88a
Author: Daniel Kulp <dkulp@apache.org>
AuthorDate: Mon Jul 17 15:08:10 2017 -0400

    [AVRO-2051] Remove synchronization for JsonProperties.getJsonProp
    Fixes #245
---
 .../main/java/org/apache/avro/JsonProperties.java  | 92 +++++++++++++++++++---
 .../src/main/java/org/apache/avro/Protocol.java    | 10 +--
 .../avro/src/main/java/org/apache/avro/Schema.java | 28 +++----
 3 files changed, 101 insertions(+), 29 deletions(-)

diff --git a/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java b/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java
index 3844073..2fca58d 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/JsonProperties.java
@@ -17,15 +17,23 @@
  */
 package org.apache.avro;
 
+import java.util.AbstractSet;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Queue;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ConcurrentMap;
+
 import java.io.IOException;
 
 import org.apache.avro.util.internal.Accessor;
 import org.apache.avro.util.internal.Accessor.JsonPropertiesAccessor;
+import org.apache.avro.reflect.MapEntry;
 import org.apache.avro.util.internal.JacksonUtils;
 
 import com.fasterxml.jackson.core.JsonGenerator;
@@ -129,7 +137,45 @@ public abstract class JsonProperties {
   /** A value representing a JSON <code>null</code>. */
   public static final Null NULL_VALUE = new Null();
 
-  Map<String,JsonNode> props = new LinkedHashMap<>(1);
+  // use a ConcurrentHashMap for speed and thread safety, but keep a Queue of the entries
to maintain order
+  // the queue is always updated after the main map and is thus is potentially a subset of
the map.
+  // By making props private, we can control access and only implement/override the methods
+  // we need.  We don't ever remove anything so we don't need to implement the clear/remove
functionality.
+  // Also, we only ever ADD to the collection, never changing a value, so putWithAbsent is
the
+  // only modifier
+  private ConcurrentMap<String,JsonNode> props = new ConcurrentHashMap<String,JsonNode>()
{
+    private static final long serialVersionUID = 1L;
+    private Queue<MapEntry<String, JsonNode>> propOrder = new ConcurrentLinkedQueue<MapEntry<String,
JsonNode>>();
+    public JsonNode putIfAbsent(String key,  JsonNode value) {
+      JsonNode r = super.putIfAbsent(key, value);
+      if (r == null) {
+        propOrder.add(new MapEntry<String, JsonNode>(key, value));
+      }
+      return r;
+    }
+    public Set<Map.Entry<String, JsonNode>> entrySet() {
+      return new AbstractSet<Map.Entry<String, JsonNode>>() {
+        @Override
+        public Iterator<Map.Entry<String, JsonNode>> iterator() {
+          return new Iterator<Map.Entry<String, JsonNode>>() {
+            Iterator<MapEntry<String, JsonNode>> it = propOrder.iterator();
+            @Override
+            public boolean hasNext() {
+              return it.hasNext();
+            }
+            @Override
+            public java.util.Map.Entry<String, JsonNode> next() {
+              return it.next();
+            }
+          };
+        }
+        @Override
+        public int size() {
+          return propOrder.size();
+        }
+      };
+    }
+  };
 
   private Set<String> reserved;
 
@@ -150,7 +196,8 @@ public abstract class JsonProperties {
    * Returns the value of the named property in this schema.
    * Returns <tt>null</tt> if there is no property with that name.
    */
-  synchronized JsonNode getJsonProp(String name) {
+  @Deprecated
+  public JsonNode getJsonProp(String name) {
     return props.get(name);
   }
 
@@ -158,7 +205,7 @@ public abstract class JsonProperties {
    * Returns the value of the named property in this schema.
    * Returns <tt>null</tt> if there is no property with that name.
    */
-  public synchronized Object getObjectProp(String name) {
+  public Object getObjectProp(String name) {
     return JacksonUtils.toObject(props.get(name));
   }
 
@@ -175,23 +222,37 @@ public abstract class JsonProperties {
     addProp(name, TextNode.valueOf(value));
   }
 
-  synchronized void addProp(String name, JsonNode value) {
+  /**
+   * Adds a property with the given name <tt>name</tt> and
+   * value <tt>value</tt>. Neither <tt>name</tt> nor <tt>value</tt>
can be
+   * <tt>null</tt>. It is illegal to add a property if another with
+   * the same name but different value already exists in this schema.
+   *
+   * @param name The name of the property to add
+   * @param value The value for the property to add
+   * @deprecated use {@link #addProp(String, Object)}
+   */
+  @Deprecated
+  public void addProp(String name, JsonNode value) {
     if (reserved.contains(name))
       throw new AvroRuntimeException("Can't set reserved property: " + name);
 
     if (value == null)
       throw new AvroRuntimeException("Can't set a property to null: " + name);
 
-    JsonNode old = props.get(name);
-    if (old == null)
-      props.put(name, value);
-    else if (!old.equals(value))
+    JsonNode old = props.putIfAbsent(name,  value);
+    if (old != null && !old.equals(value)) {
       throw new AvroRuntimeException("Can't overwrite property: " + name);
+    }
   }
-
-  public synchronized void addProp(String name, Object value) {
+  public void addProp(String name, Object value) {
     addProp(name, JacksonUtils.toJsonNode(value));
   }
+  public void putAll(JsonProperties np) {
+    for (Map.Entry<? extends String, ? extends JsonNode> e : np.props.entrySet())
+      addProp(e.getKey(), e.getValue());
+  }
+
 
   /**
    * Adds all the props from the specified json properties.
@@ -241,4 +302,15 @@ public abstract class JsonProperties {
       gen.writeObjectField(e.getKey(), e.getValue());
   }
 
+
+  int propsHashCode() {
+    return props.hashCode();
+  }
+  boolean propsEqual(JsonProperties np) {
+    return props.equals(np.props);
+  }
+  public boolean hasProps() {
+    return !props.isEmpty();
+  }
+
 }
diff --git a/lang/java/avro/src/main/java/org/apache/avro/Protocol.java b/lang/java/avro/src/main/java/org/apache/avro/Protocol.java
index c5800a7..f381dd6 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/Protocol.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/Protocol.java
@@ -148,11 +148,11 @@ public class Protocol extends JsonProperties {
       Message that = (Message)o;
       return this.name.equals(that.name)
         && this.request.equals(that.request)
-        && props.equals(that.props);
+        && propsEqual(that);
     }
 
     public int hashCode() {
-      return name.hashCode() + request.hashCode() + props.hashCode();
+      return name.hashCode() + request.hashCode() + propsHashCode();
     }
 
     public String getDoc() { return doc; }
@@ -237,7 +237,7 @@ public class Protocol extends JsonProperties {
    */
   public Protocol(Protocol p) {
     this(p.getName(), p.getDoc(), p.getNamespace());
-    props.putAll(p.props);
+    putAll(p);
   }
 
   public Protocol(String name, String doc, String namespace) {
@@ -320,12 +320,12 @@ public class Protocol extends JsonProperties {
       && this.namespace.equals(that.namespace)
       && this.types.equals(that.types)
       && this.messages.equals(that.messages)
-      && this.props.equals(that.props);
+      && this.propsEqual(that);
   }
 
   public int hashCode() {
     return name.hashCode() + namespace.hashCode()
-      + types.hashCode() + messages.hashCode() + props.hashCode();
+      + types.hashCode() + messages.hashCode() + propsHashCode();
   }
 
   /** Render this as <a href="http://json.org/">JSON</a>.*/
diff --git a/lang/java/avro/src/main/java/org/apache/avro/Schema.java b/lang/java/avro/src/main/java/org/apache/avro/Schema.java
index 6c63ddc..e08331e 100644
--- a/lang/java/avro/src/main/java/org/apache/avro/Schema.java
+++ b/lang/java/avro/src/main/java/org/apache/avro/Schema.java
@@ -138,7 +138,7 @@ public abstract class Schema extends JsonProperties {
 
   int hashCode = NO_HASHCODE;
 
-  @Override void addProp(String name, JsonNode value) {
+  @Override public void addProp(String name, JsonNode value) {
     super.addProp(name, value);
     hashCode = NO_HASHCODE;
   }
@@ -350,7 +350,7 @@ public abstract class Schema extends JsonProperties {
   }
 
   void toJson(Names names, JsonGenerator gen) throws IOException {
-    if (props.size() == 0) {                      // no props defined
+    if (!hasProps()) {                      // no props defined
       gen.writeString(getName());                 // just write name
     } else {
       gen.writeStartObject();
@@ -369,7 +369,7 @@ public abstract class Schema extends JsonProperties {
     if (!(o instanceof Schema)) return false;
     Schema that = (Schema)o;
     if (!(this.type == that.type)) return false;
-    return equalCachedHash(that) && props.equals(that.props);
+    return equalCachedHash(that) && propsEqual(that);
   }
   public final int hashCode() {
     if (hashCode == NO_HASHCODE)
@@ -377,7 +377,7 @@ public abstract class Schema extends JsonProperties {
     return hashCode;
   }
 
-  int computeHash() { return getType().hashCode() + props.hashCode(); }
+  int computeHash() { return getType().hashCode() + propsHashCode(); }
 
   final boolean equalCachedHash(Schema other) {
     return (hashCode == other.hashCode)
@@ -458,7 +458,7 @@ public abstract class Schema extends JsonProperties {
      */
     public Field(Field field, Schema schema) {
       this(field.name, schema, field.doc, field.defaultValue, field.order);
-      props.putAll(field.props);
+      putAll(field);
       if (field.aliases != null)
         aliases = new LinkedHashSet<String>(field.aliases);
     }
@@ -512,7 +512,7 @@ public abstract class Schema extends JsonProperties {
         (schema.equals(that.schema)) &&
         defaultValueEquals(that.defaultValue) &&
         (order == that.order) &&
-        props.equals(that.props);
+        propsEqual(that);
     }
     public int hashCode() { return name.hashCode() + schema.computeHash(); }
 
@@ -721,7 +721,7 @@ public abstract class Schema extends JsonProperties {
       RecordSchema that = (RecordSchema)o;
       if (!equalCachedHash(that)) return false;
       if (!equalNames(that)) return false;
-      if (!props.equals(that.props)) return false;
+      if (!propsEqual(that)) return false;
       Set seen = SEEN_EQUALS.get();
       SeenPair here = new SeenPair(this, o);
       if (seen.contains(here)) return true;       // prevent stack overflow
@@ -822,7 +822,7 @@ public abstract class Schema extends JsonProperties {
       return equalCachedHash(that)
         && equalNames(that)
         && symbols.equals(that.symbols)
-        && props.equals(that.props);
+        && propsEqual(that);
     }
     @Override
     public String getEnumDefault() { return enumDefault; }
@@ -859,7 +859,7 @@ public abstract class Schema extends JsonProperties {
       ArraySchema that = (ArraySchema)o;
       return equalCachedHash(that)
         && elementType.equals(that.elementType)
-        && props.equals(that.props);
+        && propsEqual(that);
     }
     @Override int computeHash() {
       return super.computeHash() + elementType.computeHash();
@@ -887,7 +887,7 @@ public abstract class Schema extends JsonProperties {
       MapSchema that = (MapSchema)o;
       return equalCachedHash(that)
         && valueType.equals(that.valueType)
-        && props.equals(that.props);
+        && propsEqual(that);
     }
     @Override int computeHash() {
       return super.computeHash() + valueType.computeHash();
@@ -928,7 +928,7 @@ public abstract class Schema extends JsonProperties {
       UnionSchema that = (UnionSchema)o;
       return equalCachedHash(that)
         && types.equals(that.types)
-        && props.equals(that.props);
+        && propsEqual(that);
     }
     @Override int computeHash() {
       int hash = super.computeHash();
@@ -966,7 +966,7 @@ public abstract class Schema extends JsonProperties {
       return equalCachedHash(that)
         && equalNames(that)
         && size == that.size
-        && props.equals(that.props);
+        && propsEqual(that);
     }
     @Override int computeHash() { return super.computeHash() + size; }
     void toJson(Names names, JsonGenerator gen) throws IOException {
@@ -1514,7 +1514,7 @@ public abstract class Schema extends JsonProperties {
         Schema fSchema = applyAliases(f.schema, seen, aliases, fieldAliases);
         String fName = getFieldAlias(name, f.name, fieldAliases);
         Field newF = new Field(fName, fSchema, f.doc, f.defaultValue, f.order);
-        newF.props.putAll(f.props);               // copy props
+        newF.putAll(f);               // copy props
         newFields.add(newF);
       }
       result.setFields(newFields);
@@ -1547,7 +1547,7 @@ public abstract class Schema extends JsonProperties {
       break;
     }
     if (result != s)
-      result.props.putAll(s.props);        // copy props
+      result.putAll(s);        // copy props
     return result;
   }
 


Mime
View raw message