-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sql partition aware routing[API-1862] #1167
Conversation
Concurrent map
Windows test PASSed. |
Linux test PASSed. |
hazelcast/include/hazelcast/client/sql/impl/read_optimized_lru_cache.h
Outdated
Show resolved
Hide resolved
hazelcast/include/hazelcast/client/sql/impl/read_optimized_lru_cache.h
Outdated
Show resolved
Hide resolved
Linux test FAILed. |
Windows test FAILed. |
Windows test PASSed. |
Linux test PASSed. |
Windows test PASSed. |
Linux test PASSed. |
1 similar comment
Linux test PASSed. |
Windows test FAILed. |
Linux test FAILed. |
1 similar comment
Linux test FAILed. |
Windows test PASSed. |
Windows test PASSed. |
Linux test PASSed. |
* or {@code null} if this cache contains no mapping for the key. | ||
* @returns Returns the value to which the specified key is cached | ||
*/ | ||
std::shared_ptr<V> get(const K& key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not return optional
which makes small value optimizations for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it to be consistent with SynchronizedMap, so I prefer to leave it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may have another version of synchronized map to work with optional, what do you think? They are very similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data is stored in synchronized map, and we just increment the usage count of the shared_ptr with get (which is done via atomic instruction). If we use optional, wouldn't it be expensive, because this time we will copy the data for storing it in optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this specific case, the value is an (int32 + int64_t) struct (std::shared_ptr<impl::read_optimized_lru_cache<std::string, int32_t>> partition_argument_index_cache_;
) hence, copy would not be a big problem for this specific case. We have no other use of read_optimized_lru_cache structure any other places and have no such concern at the moment. Hence, I thought that it would have been better than using shared_ptr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right, I have changed the implementation. (#1188)
This PR includes partition aware routing for SQL.
A LRU cache will be added to the SQL service. This cache will be responsible for holding key argument index of the most recently used SQL statements. If the entry is not cached, the partition id will be queried via the current connection. When the sql query result is received, the entry will be added to the cache and next pages will be queried to the connection represented by the partition id.