You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/06/29 22:22:18 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request #2183: Use "ID" instead of "name" for tables in new RPCs
ctubbsii opened a new pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183
Use TableId and NamespaceId instead of tableName and namespaceName in
new RPCs for 2.1
* Standardize naming conventions for table and namespace IDs and names
in thrift IDL files
* Create Tid ("thrift ID") wrapper for strings, to avoid string types
directly in thrift IDL
* Change client code where necessary to pass ID types instead of string
names, by looking up the ID right away, and using that in the RPC
methods, and change corresponding server code to eliminate unnecessary
ID lookups
* Eliminate redundant name validation in client code (it's done in the
ID lookup)
--
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] Manno15 commented on a change in pull request #2183: Use "ID" instead of "name" for tables in new RPCs
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183#discussion_r662430331
##########
File path: core/src/main/thrift/client.thrift
##########
@@ -22,6 +22,11 @@ namespace cpp org.apache.accumulo.core.clientImpl.thrift
include "security.thrift"
include "trace.thrift"
+// Thrift Id, a wrapper for AbstractId over thrift
+struct Tid {
Review comment:
I am in favor of the TTable/TNamespace idea that contains a pair of ID/Name.
--
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] ctubbsii commented on a change in pull request #2183: Use "ID" instead of "name" for tables in new RPCs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183#discussion_r662423662
##########
File path: core/src/main/thrift/client.thrift
##########
@@ -22,6 +22,11 @@ namespace cpp org.apache.accumulo.core.clientImpl.thrift
include "security.thrift"
include "trace.thrift"
+// Thrift Id, a wrapper for AbstractId over thrift
+struct Tid {
Review comment:
I wasn't sure if I should even keep this type at all, or if I should make two separate types, TTableId and TNamespaceId, which corresponding utility methods, to further disambiguate. I was also considering TTable and TNamespace types that would have an ID/Name pair, to make these useful to carry name information for log messages so that way we don't have to do a name lookup on the server side for a log message, and so we could see the name the client thought the table was at the time it did the ID lookup, in case the table gets renamed before the log message is printed.
In short, there's room to grow here, but I'm not 100% sure which is the best direction.
--
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] dlmarion commented on a change in pull request #2183: Use "ID" instead of "name" for tables in new RPCs
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183#discussion_r662204755
##########
File path: core/src/main/thrift/client.thrift
##########
@@ -22,6 +22,11 @@ namespace cpp org.apache.accumulo.core.clientImpl.thrift
include "security.thrift"
include "trace.thrift"
+// Thrift Id, a wrapper for AbstractId over thrift
+struct Tid {
Review comment:
or `idType`
--
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] ctubbsii commented on a change in pull request #2183: Use "ID" instead of "name" for tables in new RPCs
Posted by 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
[GitHub] [accumulo] ctubbsii commented on pull request #2183: Use "ID" instead of "name" for tables in new RPCs
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183#issuecomment-876804010
I'm going to take another pass at this, using `TTable` and `TNamespace` instead.
--
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] ctubbsii closed pull request #2183: Use "ID" instead of "name" for tables in new RPCs
Posted by GitBox <gi...@apache.org>.
ctubbsii closed pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183
--
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] milleruntime commented on a change in pull request #2183: Use "ID" instead of "name" for tables in new RPCs
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2183:
URL: https://github.com/apache/accumulo/pull/2183#discussion_r661431696
##########
File path: core/src/main/thrift/client.thrift
##########
@@ -22,6 +22,11 @@ namespace cpp org.apache.accumulo.core.clientImpl.thrift
include "security.thrift"
include "trace.thrift"
+// Thrift Id, a wrapper for AbstractId over thrift
+struct Tid {
Review comment:
What about `TAbstractId`? There are places in the code where we use "tid" for transactionId.
##########
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:
Will this cause issues with older RPC calls? Maybe that's OK to eliminate the ambiguity of "table" since to make it backwards compatible we would have to keep it around.
--
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