[ https://issues.apache.org/jira/browse/CASSANDRA-15393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17189237#comment-17189237 ]
Marcus Eriksson commented on CASSANDRA-15393: --------------------------------------------- First pass done, LGTM in general, I'll try to get a second pass done this week. *Issues* {{CounterContext.java}} * {{hasLegacyShards(..)}} {{totalCount}} looks wrong, should probably be {{int totalCount = (accessor.size(context) - headerLength(context, accessor)) / STEP_LENGTH;}} {{ByteBufferAccessor.java}} * {{digest(..)}} needs to add {{value.position}} to {{offset}} {{DynamicCompositeType.java}} * in {{validateComparator(..)}}, adds {{1}} to {{offset}} after reading a {{short}} *Nits* General stuff * Slightly inconsistent use of {{TypeSizes.INT_SIZE}} / {{TypeSize.sizeof(int)}} / hard coded {{4}}, I'd probably prefer being explicit and use X_SIZE, mostly since we represent shorts etc in ints * A few places added the {{left}} / {{right}} / {{accessorL}} / {{accessorR}} parameters, but then have {{offset1/offset2/size1/size2}} - maybe rename these to {{offsetL/offsetR}} etc {{CQL3Type.java}} * {{generateXCQLLiteral}}: no need to return the offset * unused import ByteBufferUtil {{AbstractClusteringPrefix.java}} * maybe bring up {{size()}} and {{get(i)}} here (as {{getRawValues().length}} and {{getRawValues()[i]}}), and only leave them overridden in {{NativeClustering}} {{ClusteringPrefix.java}} * {{isBottom}} & {{isTop}} can use {{size()}} instead of {{getRawValues().length}} {{ArrayClusteringBound.java}} + {{ArrayClusteringBoundary.java}} + {{BufferClusteringBound.java}} + {{BufferClusteringBoundary.java}} * unsharedHeapSizeExcludingData unused {{AbstractType.java}} * {{readArray}} unused {{ByteBufferAccessor.java}} * in {{compare(..)}} - instead of casting to a {{ByteBuffer}} maybe use {{accessor.toBuffer(right)}} (same in {{ByteArrayAccessor}}) {{DynamicCompositeType.java}} * no need to {{getComparatorSize}} in {{getComparator(int i, VL left, ValueAccessor<VL> accessorL, VR right, ValueAccessor<VR> accessorR, int offset1, int offset2)}} {{MapType.java}} * don't use {{V}} as the type parameter in {{referencesUserType}} - quite confusing as it clashes with the {{MapType<K, V>}} type param {{ValueAccessor.java}} * why have {{compare}} / {{compareUnsigned}} with the same parameters? > Add byte array backed cells > --------------------------- > > Key: CASSANDRA-15393 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15393 > Project: Cassandra > Issue Type: Sub-task > Components: Local/Compaction > Reporter: Blake Eggleston > Assignee: Blake Eggleston > Priority: Normal > Fix For: 4.0-beta > > Time Spent: 20m > Remaining Estimate: 0h > > We currently materialize all values as on heap byte buffers. Byte buffers > have a fairly high overhead given how frequently they’re used, and on the > compaction and local read path we don’t do anything that needs them. Use of > byte buffer methods only happens on the coordinator. Using cells that are > backed by byte arrays instead in these situations reduces compaction and read > garbage up to 22% in many cases. -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org