Skip to content

Conversation

@ptupitsyn
Copy link
Contributor

@ptupitsyn ptupitsyn commented Sep 9, 2021

  • Implement KeyValueBinaryView for Java thin client (ignite.tables().table(..).kvView()).
  • Update protocol: return only value columns of the tuple when the key is known. For example, TUPLE_GET passes the key to the server, and server used to return full tuple: key and value columns. It is not necessary to pass the key back, and the server won't do that anymore.
  • On the protocol and server levels there is no difference between Table and KvView APIs - the same operations are used for both. The difference is only on the client side - Table returns all columns in a single Tuple, and KvView splits key/val columns into separate Tuples.

IEP updated accordingly: https://cwiki.apache.org/confluence/display/IGNITE/IEP-76+Thin+Client+Protocol+for+Ignite+3.0

@ptupitsyn ptupitsyn requested a review from AMashenkov September 9, 2021 16:57
@ptupitsyn ptupitsyn self-assigned this Sep 9, 2021
@ptupitsyn ptupitsyn marked this pull request as draft September 9, 2021 16:58
@ptupitsyn ptupitsyn removed the request for review from AMashenkov September 9, 2021 16:59
@ptupitsyn ptupitsyn marked this pull request as ready for review September 10, 2021 07:24
* limitations under the License.
*/

package org.apache.ignite.client.proto;
Copy link
Member

@AMashenkov AMashenkov Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean 'proto' is a public package?
Let's move it into 'internal' package or write javadoc to all enum values. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

var tuple = readTuple(in, table, true);

return table.getAndDeleteAsync(tuple).thenAccept(resTuple -> writeTuple(out, resTuple));
return table.getAndDeleteAsync(tuple).thenAccept(resTuple -> writeTuple(out, resTuple, TuplePart.VAL));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to reuse this (and similar) methods in Table binary view?
I think user expects TableView.get(t) will return full row (key+value parts), but KvView will return "value" part only, thought.
Shouldn't TuplePart be passed from outside?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to reuse this (and similar) methods in Table binary view?

This is a server-side handler. Table and KvView APIs only differ in the way they return data to the user, but the data is the same. There is nothing specific to Table or KvView on the protocol level - all operations are shared.

For Get* operations we only return the value part from the server, because the client already knows the key. For the Table view, we combine returned value columns with known key columns to create the full row and return it to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR description to clarify this point.

public enum TuplePart {
KEY,
VAL,
KEY_AND_VAL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about 'contains' operation? It don't require any key or value to be returned.
It's ok if you plan to optimize it later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a separate ticket for this part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, added ClientOp.TUPLE_CONTAINS_KEY.

Copy link
Contributor

@isapego isapego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@ptupitsyn ptupitsyn merged commit 551a9f0 into apache:main Sep 13, 2021
@ptupitsyn ptupitsyn deleted the ignite-15361 branch September 13, 2021 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants