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

Vladimir Ozerov commented on IGNITE-7421:
-----------------------------------------

[~kukushal], my comments:
1) By convention we always set {{serialVersionUID}} to {{0L}}.
2) I do not recommend to use {{Objects.hash(...)}} method because it is vararg 
and creates byte array underneath every time. It is better to calculate hash 
code by hand using standard {{31 * x + y}} algorithm. We should discourage 
ourselves from using helpers with hidden effects for simple code parts. 
3) {{SslProtocol and SslMode}} - as these things are specific to thin client, 
let's move them to thin client package. 
4) {{SslProtocol}} - if we have only TLS mode (i.e. SSL is considered totally 
deprecated), why would we need this enum at all? Let's remove it along with 
relevant properties altogether for the sake of simplicity.
5) {{ClientCacheConfiguration}} is not top-level thing, and hence should be 
located inside {{client}} package, not inside 
{{org.apache.ignite.configuration}}
6) {{ClientCacheConfiguration}} - collection-based getter/setters should be 
symmetircal. I.e. they either accept array (vararg) and return array, or accept 
collection and return collection. Arrays is considered better practice at the 
moment.
7) {{IgniteClientConfiguration.addrs}} - we should not resolve addresses here, 
neither we should expose resolved address with public methods. Instead, 
resolution should happen in runtime when connection attempt is performed. 
Otherwise your code is not responsive to runtime changes to network 
configuration. 
8) Taking in count p.7, {{TcpAddressConfigLines}} appears to me a dangerous 
abstraction. I would rather convert it to a static stateless method inside 
{{IgniteUtils}} which is called in runtime in during connection attempt. 
9) {{TcpDiscoveryVmIpFinder}} - I would recommend to revert the changes. It is 
considered bad practice to change classes which is unrelated to the ticket. If 
refactoring is needed, it should be performed in separate ticket. This approach 
minimizes impact of certain changes and simplified review and testing. In this 
particular case - {{TcpDiscoveryVmIpFinder}} is central class, very sensitive 
to any changes. Any change introduced here may affect the whole project. 
Definitely not the right move provided that current ticket is focused on thin 
client and should not affect core logic in general.
10) Let's rename {{org.apache.ignite.thinclient}} package to 
{{org.apache.ignite.client}}. Word {{thin}} doesn't add much value, because our 
non-thin client is already considered deprecated and will be removed soon.
11) {{IgniteUnavailableException}} - I would rename it to 
{{ClientConnectionException}}
12) {{IgniteConfigurationException}} - not clear why we need it, provided p.7, 
p.8 and p.10. Appears to be a wrong abstraction to me.
13) {{IgniteBinaryMarshaller}} - there should be no static. In principle user 
may have different clients connected to different grids. In general, any static 
state is discouraged in Ignite. Instead, it is better to have a singleton 
marshaller *per-client*.

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

Reply via email to