You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/12/11 23:01:20 UTC
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #697: Feature/asio
pdxcodemonkey commented on a change in pull request #697:
URL: https://github.com/apache/geode-native/pull/697#discussion_r541338599
##########
File path: cppcache/src/Connector.hpp
##########
@@ -37,24 +37,28 @@ constexpr std::chrono::milliseconds DEFAULT_READ_TIMEOUT = DEFAULT_TIMEOUT;
constexpr std::chrono::milliseconds DEFAULT_WRITE_TIMEOUT = DEFAULT_TIMEOUT;
-class Connector {
+class APACHE_GEODE_EXPORT Connector {
Review comment:
Why did you need to export this? Only internal headers should be in the /src directory, so this should either not be APACHE_GEODE_EXPORT, or the header belongs in /include/geode with the other public headers, hopefully the former. Please advise.
##########
File path: cppcache/src/Connector.hpp
##########
@@ -63,45 +67,88 @@ class Connector {
*
* @param b the buffer into which the data is read.
* @param len the number of bytes to read.
- * @param waitSeconds the number of seconds to allow the read to
- * complete.
+ * @param timeout time to allow the read to complete.
* @return the total number of bytes read into the buffer, or
* <code>-1</code> if an error was encountered.
* @exception GeodeIOException, TimeoutException, IllegalArgumentException,
* OutOfMemoryException.
*/
virtual size_t receive(char *b, size_t len,
- std::chrono::microseconds waitSeconds) = 0;
+ std::chrono::milliseconds timeout) = 0;
+ /**
+ * Reads an undetermined number of bytes of data and stores them into the
+ * buffer array <code>b</code>. The number of bytes actually read is returned
+ * as an integer. This method blocks until <code>len</code> bytes of data is
+ * read, the timeout expires or an exception is thrown.
+ *
+ * <p> If <code>b</code> is <code>null</code> an
+ * <code>IllegalArgumentException</code>
+ * is thrown.
+ *
+ * <p> The <code>read(b)</code> method for class <code>InputStream</code>
+ * has the same effect as: <pre><code> read(b, 0, b.length) </code></pre>
+ *
+ * @param b the buffer into which the data is read.
+ * @param timeout time to allow the read to complete.
+ * @return the total number of bytes read into the buffer, or
+ * <code>-1</code> if an error was encountered.
+ * @exception GeodeIOException, IllegalArgumentException,
+ * OutOfMemoryException.
+ */
+ virtual size_t receive_nothrowiftimeout(
+ char *b, size_t len, std::chrono::milliseconds timeout) = 0;
/**
* Writes <code>len</code> bytes from the specified byte array
* to the underlying output stream.
*
* @param b the data.
* @param len the number of bytes to write.
- * @param waitSeconds the number of seconds to allow the write to
- * complete.
+ * @param timeout time to allow the write to complete.
* @return the actual number of bytes written.
* @exception GeodeIOException, TimeoutException, IllegalArgumentException.
*/
virtual size_t send(const char *b, size_t len,
- std::chrono::microseconds waitSeconds) = 0;
+ std::chrono::milliseconds timeout) = 0;
/**
- * Initialises the connection.
+ * Returns local port for this TCP connection
*/
- virtual void init() = 0;
+ virtual uint16_t getPort() = 0;
/**
- * Closes the connection.
+ * Writes an array of a known size to the underlying output stream.
+ *
+ * @param array A C-style stack array. Be weary of arrays that have been
+ * decayed into pointers, they won't compile.
+ * @return The number of bytes written. Don't get confused: this is not the
+ * number of elements in the array written.
+ * @exception GeodeIOException, TimeoutException
*/
- virtual void close() = 0;
+ template <typename T, size_t size>
+ size_t send(const T (&array)[size], std::chrono::milliseconds timeout) {
+ return send(reinterpret_cast<const char *>(array), sizeof(T) * size,
+ timeout);
+ }
/**
- * Returns local port for this TCP connection
+ * Reads an array of a known size from the underlying input stream. If the
+ * number of bytes read is not equal to the size of the array, no exception
+ * will be thrown.
+ *
+ * @param array A C-style stack array. Be weary of arrays that have been
+ * decayed into pointers, they won't compile.
+ * @return The number of bytes written. Don't get confused: this is not the
+ * number of elements in the array written.
+ * @exception GeodeIOException
*/
- virtual uint16_t getPort() = 0;
+ template <typename T, size_t size>
+ size_t receive(T (&array)[size], std::chrono::milliseconds timeout) {
+ return receive_nothrowiftimeout(reinterpret_cast<char *>(array),
+ sizeof(T) * size, timeout);
+ }
Review comment:
This function body should go in the .cpp file. Yes, it's a trivial function, but we have had all kinds of problems with inlining of instance methods in public headers, so the unwritten rule is no inline functions in public headers. Of course, if you end up not exporting the class, this is moot.
##########
File path: cppcache/src/PoolFactory.cpp
##########
@@ -172,14 +172,14 @@ PoolFactory& PoolFactory::setServerGroup(std::string group) {
}
PoolFactory& PoolFactory::addLocator(const std::string& host, int port) {
- addCheck(host, port);
+ // addCheck(host, port);
m_attrs->addLocator(host, port);
m_addedServerOrLocator = true;
return *this;
}
PoolFactory& PoolFactory::addServer(const std::string& host, int port) {
- addCheck(host, port);
+ // addCheck(host, port);
Review comment:
Pls just delete the commented out code, no need to keep it here
##########
File path: cppcache/src/TcrConnection.hpp
##########
@@ -120,43 +109,12 @@ class TcrConnection {
* @param ports List of local ports for connections to endpoint
*/
bool initTcrConnection(
- TcrEndpoint* endpointObj, const char* endpoint,
+ std::shared_ptr<TcrEndpoint> endpointObj,
Review comment:
YES!!! So much better/readable with the shared ptr, thx
##########
File path: defaultSystem/geode.properties
##########
@@ -28,8 +28,8 @@
#
## Log file config
#
-#log-file=geode_native.log
-#log-level=config
+log-file=geode_native.log
+log-level=debug
Review comment:
Not sure if debug is the correct 'default' level...
##########
File path: cppcache/src/DistributedSystem.hpp
##########
@@ -108,7 +107,6 @@ class APACHE_GEODE_EXPORT DistributedSystem {
friend class CacheRegionHelper;
friend class DistributedSystemImpl;
- friend class TcrConnection;
};
Review comment:
So happy to see one fewer broken "friend" declaration, thanks!
##########
File path: tests/cpp/fwklib/TcpIpc.cpp
##########
@@ -106,21 +106,6 @@ void TcpIpc::init(int32_t sockBufferSize) {
ACE_OS::signal(SIGPIPE, SIG_IGN); // Ignore broken pipe
Review comment:
Do we need the jks files above in that directory? If so, have the existing ones under /sni-test-config been removed? I really don't want to go back to a situation where we have multiple copies of the same key(s) checked into the tree.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org