Hi Val, Thanks for the comments, please see below:
> Users will identify tables using names, they will never use IDs Ok, let's keep it this way. > Sounds like the Tuple should implement Iterable. I don't think Iterable is enough. We should have a way to get values by column index: Tuple.value(int index), where index is according to column order in schema. Combined with Tuple.schema(), it will provide an efficient way to consume data - users will be able to read columns in any order, skip some of them, etc. This is a common pattern in APIs like ODBC or System.Data [1], which we'll need to implement on top of our thin clients later. > However, whether a user might > need to get a table schema for a particular version, I'm not sure. Do you > have a use case in mind for this? If not, I would keep this internal Ok, we can keep the Table.schema(ver) method internal, as long as Table.schemas() is public and includes schema versions. > We already have async counterparts for all the methods where this is applicable IgniteTables.createTable(), dropTable(), tables(), table() do not have async counterparts, while internally they perform blocking wait on some futures. [1] https://docs.microsoft.com/en-us/dotnet/api/system.data.idatareader?view=net-5.0 On Wed, Jun 30, 2021 at 4:51 AM Valentin Kulichenko < valentin.kuliche...@gmail.com> wrote: > Hi Pavel, > > Please see my comments below. > > -Val > > On Tue, Jun 29, 2021 at 2:23 PM Pavel Tupitsyn <ptupit...@apache.org> > wrote: > > > Igniters, > > > > While working on "IEP-76 Thin Client Protocol for Ignite 3.0" [1] (to be > > discussed separately), the following suggestions for the Table API came > up: > > > > 1. Expose table IDs: sending table name with every operation (e.g. GET) > is > > inefficient, string serialization is expensive by itself and names can be > > long. > > - Table.id() > > - IgniteTables.table(UUID) > > - IgniteTables.dropTable(UUID) > > > > I don't think this should be a part of the public API. Users will identify > tables using names, they will never use IDs. As an internal optimization > though - sure, we can have that. Sounds similar to the cache ID in 2.x. > > > > > > 2. Expose tuple schemas: to reduce payload size when sending tuples to > the > > client, we'll write only the schema version and column values, then the > > client can retrieve and cache schemas (ordered set of columns per > version). > > - Tuple.schema() > > - Table.schemas() > > - Table.schema(ver) > > > > Exposing the schema of a tuple makes sense. However, whether a user might > need to get a table schema for a particular version, I'm not sure. Do you > have a use case in mind for this? If not, I would keep this internal as > well. > > > > > > 3. Expose tuple values as a collection: to serialize tuples efficiently > > (see p2) we need an API to get all values at once. Right now the only API > > is to get values by column name, which involves a HashMap lookup on every > > call. > > - Tuple.values() > > > > Sounds like the Tuple should implement Iterable. > > > > > > 4. Simplify createTable API: use POJO-based configuration. > > Creating a Consumer when some properties are optional seems to be > > non-trivial. > > > > Yes, it's currently convoluted for sure. To my knowledge, there are plans > to improve this, but I will let other folks chime in with more details. > > > > > > 5. Add async methods to IgniteTables API (all methods are async inside > > anyway) > > > > We already have async counterparts for all the methods where this is > applicable. E.g., for TableView#get, there is TableView#getAsync. Is there > anything else that you propose to add? > > > > > > > > Thoughts, objections? > > > > > > [1] > > > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-76+Thin+Client+Protocol+for+Ignite+3.0 > > > > [2] > > > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-54%3A+Schema-first+Approach > > >
