[GitHub] [accumulo] ctubbsii commented on a change in pull request #2183: Use "ID" instead of "name" for tables in new RPCs

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183#discussion_r662421201



##########
File path: core/src/main/thrift-gen-java/org/apache/accumulo/core/clientImpl/thrift/TDiskUsage.java
##########
@@ -28,18 +28,18 @@
 public class TDiskUsage implements org.apache.thrift.TBase<TDiskUsage, TDiskUsage._Fields>, java.io.Serializable, Cloneable, Comparable<TDiskUsage> {
   private static final org.apache.thrift.protocol.TStruct STRUCT_DESC = new org.apache.thrift.protocol.TStruct("TDiskUsage");
 
-  private static final org.apache.thrift.protocol.TField TABLES_FIELD_DESC = new org.apache.thrift.protocol.TField("tables", org.apache.thrift.protocol.TType.LIST, (short)1);
+  private static final org.apache.thrift.protocol.TField TABLE_NAMES_FIELD_DESC = new org.apache.thrift.protocol.TField("tableNames", org.apache.thrift.protocol.TType.LIST, (short)1);
   private static final org.apache.thrift.protocol.TField USAGE_FIELD_DESC = new org.apache.thrift.protocol.TField("usage", org.apache.thrift.protocol.TType.I64, (short)2);
 
   private static final org.apache.thrift.scheme.SchemeFactory STANDARD_SCHEME_FACTORY = new TDiskUsageStandardSchemeFactory();
   private static final org.apache.thrift.scheme.SchemeFactory TUPLE_SCHEME_FACTORY = new TDiskUsageTupleSchemeFactory();
 
-  public @org.apache.thrift.annotation.Nullable java.util.List<java.lang.String> tables; // required
+  public @org.apache.thrift.annotation.Nullable java.util.List<java.lang.String> tableNames; // required
   public long usage; // required
 
   /** The set of fields this struct contains, along with convenience methods for finding and manipulating them. */
   public enum _Fields implements org.apache.thrift.TFieldIdEnum {
-    TABLES((short)1, "tables"),
+    TABLE_NAMES((short)1, "tableNames"),

Review comment:
       I spent some time reading various guides on Thrift RPC compatibility. From everything I can tell, the RPC compatibility depends only on the data type (which I only changed in stuff that's new in 2.1) and the numeric field identifier (which I didn't change) to match up the fields for deserialization. Field names don't matter. However, RPC method names (which I didn't change) do matter.
   
   What is broken is class path dependencies... which is fine... because you shouldn't be mixing and matching Accumulo jars from different versions, and the Thrift IDL is not public API for users, so they shouldn't have any references to these in their code anyway.
   
   That said, I'm pretty sure we've already broken client-server RPC compatibility between 2.0 and 2.1, so it is not a problem to introduce additional changes that change the RPC layer a bit. But, I avoided making any such changes here, before taking a closer to look to ensure compatibility was already broken and there would be no additional harm.




-- 
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