[jira] [Comment Edited] (IGNITE-7421) Thin client Java API - data grid API
[
https://issues.apache.org/jira/browse/IGNITE-7421?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16385194#comment-16385194
]
Alexey Kukushkin edited comment on IGNITE-7421 at 3/4/18 3:38 PM:
--
[~vozerov]: Vladimir, I implemented all your comments above except #12:
"_{{IgniteClient.nonQuery}} - let's remove this for now_"
The method was added for two purposes:
# Allow executing SQL statements outside the cache context. In other words,
having this method inside IgniteClient (as opposed to ClientCache#query) allows
running DDL without binding to any cache first.
# it is called "nonQuery" (instead of "query") since the "query" methods are
all lazy: you have to either iterate the returned cursor or call
Cursor#getAll() to have the SQL statement actually executed. nonQuery semantics
is eager: it executes the passed statement right away. If we replace it with a
"query" method the user would have to write DDL like this:
"ignite.query("CREATE TABLE TBL").getAll()". That "getAll" is really confusing
for DDL.
This is why I still think we need IgniteClient#nonQury(String)
I had to recreate code review in Upsource. The new core review is
https://reviews.ignite.apache.org/apache-ignite/review/IG-CR-25
was (Author: kukushal):
[~vozerov]: Vladimir, I implemented all your comments above except #12:
"_{{IgniteClient.nonQuery}} - let's remove this for now_"
The method was added for two purposes:
# Allow executing SQL statements outside the cache context. In other words,
having this method inside IgniteClient (as opposed to ClientCache#query) allows
running DDL without binding to any cache first.
# it is called "nonQuery" (instead of "query") since the "query" methods are
all lazy: you have to either iterate the returned cursor or call
Cursor#getAll() to have the SQL statement actually executed. nonQuery semantics
is eager: it executes the passed statement right away. If we replace it with a
"query" method the user would have to write DDL like this:
"ignite.query("CREATE TABLE TBL").getAll()". That "getAll" is really confusing
for DDL.
This is why I still think we need IgniteClient#nonQury(String)
> 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
> 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)
[jira] [Comment Edited] (IGNITE-7421) Thin client Java API - data grid API
[
https://issues.apache.org/jira/browse/IGNITE-7421?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16382021#comment-16382021
]
Vladimir Ozerov edited comment on IGNITE-7421 at 3/1/18 1:57 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}},
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.: {{ClientException}}, {{ClientConfiguration}}, 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
[jira] [Comment Edited] (IGNITE-7421) Thin client Java API - data grid API
[
https://issues.apache.org/jira/browse/IGNITE-7421?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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}},
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
