ignite-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vladimir Ozerov (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (IGNITE-7421) Thin client Java API - data grid API
Date Thu, 01 Mar 2018 13:54:00 GMT

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

Mime
View raw message