[jira] [Comment Edited] (IGNITE-7421) Thin client Java API - data grid API

["Vladimir Ozerov (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/IGNITE-7421?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16382021#comment-16382021 ] 

Vladimir Ozerov edited comment on IGNITE-7421 at 3/1/18 1:53 PM:
-----------------------------------------------------------------

[~kukushal], my comments:
1) {{Factory}} - let's use {{javax.cache.configuration.Factory}} instead of our own interface to avoid duplication; we already do this in many places of our configuration
2) {{IgniteClientConfiguration}}:
- by convention we put all top-level configuration classes to {{org.apache.ignite.configuration}}
- we set {{tcpNoDelay}} to {{true}} by default in all other places, let's do the same here
- let's set {{sslProto}} to {{TLS}} by default explicitly instead of doing this somewhere deep inside internal classes
- {{credentialsProvider}} factory looks like an overkill for me; plain username and password strings would be enough, the easier - the better (that is, even {{Credentials}} class could be removed)
- during JDBC/ODBC development we revealed that setting socket buffer sizes to OS defaults are typically bad idea because these buffers are too small. Let's set them to 32Kb by default (we do the same for {{TcpCommunicationSpi}})
- there should be only default constructor; the rest things are set through settter, this is our product-wide conventions
- addresses should be stored as {{String[]}}, not {{List<InetSocketAddress>}}, relevant setter should accept vararg ({{setAddresses(String...)}}) - this is proven to be more convenient for users; all further address resolution should happen outside of configuration class
- configuration should be {{Serializable}} for the sake of user convenience
- I think it is better to expose {{sslProtocol}} as enum rather than as string; without this user will not know the list of possible values without reading documentation
- instead of having different setters for host and port, there should be only one setter {{setAddresses}}. This is how we defined endpoints in other parts of the system. There should be no defaults - at least one address should always be provided explicitly

3) {{Credentials}} - not sure we need it at all
4) {{SystemEvent}}  - we are going to design common error code glossary soon. Let's avoid doing this for separate components without seeing the whole picture. String error message would be enough for now
5) {{ProtocolVersion}} is purely internal thing and should not be exposed to public API
6) {{SslMode}} - let's rename members to {{REQUIRE*D*}} and {{DISABLE*D*}}
7) I do not think we need {{IgniteClientError}}, {{IgniteProtocolError}}, {{IgniteProtocolVersionMismatch}} and {{IgniteServerError}} for now, {{IgniteClientException}} should be enough. Moreover, they are not "errors" in general sense, but "exceptions". Instead, all these cases should be expressed through a sensible error messages.
8) We need to have common prefixes for all public classes, {{Client}} in this case; e.g.: {{ClientExcpetion}}, {{ClientConfiguratoin}}, etc.
9) {{IgniteUnavailableException}} - please confirm that exceptions from all provided addresses are tracked properly here with help of "supressed excpetion" Java feature
10) {{CacheClientConfiguration}}:
- there should be only default constructor
- class should be {{Serializable}}
- please take all defaults directly from {{CacheConfiguration}}; this way, once default is changed in CacheConfiguration, it will be applied to CacheClientConfiguration automcatically, what would help us to maintain API consistency
- {{setKeyConfiguration}}, {{setQueryEntities}} - let's use varargs here

11) {{IgniteClient.start}} - {{Ignition}} class is a better place for this method - you can create both a node and a client from a single place. This is now it is implemented in .NET (and in Hazelcast)
12) {{IgniteClient.nonQuery}} - let's remove this for now. We will design new SQL API in future and would definitely looks different and located in different place
13) {{CacheClient.cacheId}} - this should not be exposed to user API, cache ID is internal thing


was (Author: vozerov):
[~kukushal], my comments:
1) {{Factory}} - let's use {{javax.cache.configuration.Factory}} instead of our own interface to avoid duplication; we already do this in many places of our configuration
2) {{IgniteClientConfiguration}}:
- by convention we put all top-level configuration classes to {{org.apache.ignite.configuration}}
- we set {{tcpNoDelay}} to {{true}} by default in all other places, let's do the same here
- let's set {{sslProto}} to {{TLS}} by default explicitly instead of doing this somewhere deep inside internal classes
- {{credentialsProvider}} factory looks like an overkill for me; plain username and password strings would be enough, the easier - the better (that is, even {{Credentials}} class could be removed)
- during JDBC/ODBC development we revealed that setting socket buffer sizes to OS defaults are typically bad idea because these buffers are too small. Let's set them to 32Kb by default (we do the same for {{TcpCommunicationSpi}})
- there should be only default constructor; the rest things are set through settter, this is our product-wide conventions
- addresses should be stored as {{String[]}}, not {{List<InetSocketAddress>}}, relevant setter should accept vararg ({{setAddresses(String...)}}) - this is proven to be more convenient for users; all further address resolution should happen outside of configuration class
- configuration should be {{Serializable}} for the sake of user convenience
- I think it is better to expose {{sslProtocol}} as enum rather than as string; without this user will not know the list of possible values without reading documentation
- instead of having different setters for host and port, there should be only one setter {{setAddresses}}. This is how we defined endpoints in other parts of the system. There should be no defaults - at least one address should always be provided explicitly
3) {{Credentials}} - not sure we need it at all
4) {{SystemEvent}}  - we are going to design common error code glossary soon. Let's avoid doing this for separate components without seeing the whole picture. String error message would be enough for now
5) {{ProtocolVersion}} is purely internal thing and should not be exposed to public API
6) {{SslMode}} - let's rename members to {{REQUIRE*D*}} and {{DISABLE*D*}}
7) I do not think we need {{IgniteClientError}}, {{IgniteProtocolError}}, {{IgniteProtocolVersionMismatch}} and {{IgniteServerError}} for now, {{IgniteClientException}} should be enough. Moreover, they are not "errors" in general sense, but "exceptions". Instead, all these cases should be expressed through a sensible error messages.
8) We need to have common prefixes for all public classes, {{Client}} in this case; e.g.: {{ClientExcpetion}}, {{ClientConfiguratoin}}, etc.
9) {{IgniteUnavailableException}} - please confirm that exceptions from all provided addresses are tracked properly here with help of "supressed excpetion" Java feature
10) {{CacheClientConfiguration}}:
- there should be only default constructor
- class should be {{Serializable}}
- please take all defaults directly from {{CacheConfiguration}}; this way, once default is changed in CacheConfiguration, it will be applied to CacheClientConfiguration automcatically, what would help us to maintain API consistency
- {{setKeyConfiguration}}, {{setQueryEntities}} - let's use varargs here
11) {{IgniteClient.start}} - {{Ignition}} class is a better place for this method - you can create both a node and a client from a single place. This is now it is implemented in .NET (and in Hazelcast)
12) {{IgniteClient.nonQuery}} - let's remove this for now. We will design new SQL API in future and would definitely looks different and located in different place
13) {{CacheClient.cacheId}} - this should not be exposed to user API, cache ID is internal thing

> Thin client Java API - data grid API
> ------------------------------------
>
>                 Key: IGNITE-7421
>                 URL: https://issues.apache.org/jira/browse/IGNITE-7421
>             Project: Ignite
>          Issue Type: New Feature
>          Components: thin client
>            Reporter: Alexey Kukushkin
>            Assignee: Alexey Kukushkin
>            Priority: Major
>              Labels: data, java, thin
>
> Implement below Java bindings for the thin client protocol. The client configuration must support failover and encryption.
> Cache
>      JCache (limited)
>          getName(): String
>          put(key, val)
>          get(key): V
>          getAll(keys: Set): Map
>          containsKey(key): boolean
>          getAndPut(key, val): V
>          getAndReplace(key, val): V
>          getAndRemove(key): V
>          putIfAbsent
>          replace(key, val)
>          replace(key, oldVal, newVal)
>          putAll
>          clear
>          remove(key)
>          remove(key, val)
>          removeAll()
>          removeAll(keys: Set)
>          getConfiguration(clazz): Configuration
>          close()
>      size(modes: CachePeekMode...)
>      query(qry: Query): QueryCursor
>      query(qry: SqlFieldsQuery): FieldsQueryCursor<List>
>      withKeepBinary(): IgniteCache
>  Ignite
>      cache(name: String)
>      cacheNames(): Collection
>      binary(): IgniteBinary
>      createCache(name): Cache
>      getOrCreateCache(name): Cache
>      destroyCache(name)
>  Ignition
>      startClient(:ClientConfiguration): Ignite
>  ClientConfiguration(port, host, binaryConfiguration, sslConfiguration,
>  etc...)



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)