pivotal-jbarrett commented on a change in pull request #634:
URL: https://github.com/apache/geode-native/pull/634#discussion_r472527125
##########
File path: clicache/src/PoolFactory.hpp
##########
@@ -279,6 +279,20 @@ namespace Apache
/// </exception>
PoolFactory^ AddServer(String^ host, Int32 port);
+ /// <summary>
+ /// Set proxy info for SNI connection.
+ /// </summary>
+ /// <remarks>
+ /// Used for connecting via SNI proxy.
+ /// </remarks>
+ /// <param>
+ /// host the host name or ip address that the server is listening on.
+ /// </param>
+ /// <param>
+ /// port the port that the server is listening on.
+ /// </param>
+ PoolFactory^ SetSniProxy(String^ hostname, Int32 port);
Review comment:
A difference in API is a pretty big surprise to some. Especially when
you consider that .NET does expose a Socket concept in the SDK similar to Java.
Having a discussion beforehand as to why one API would deviate from another
established in an RFC helps reviewers understand the rational for the deviation.
I can only make the assumption that the deviation is a result of the C++
layer not wanting to leak `ACE::Socket` outside the internals. There are
alternative ways to do that such that you have an API similar to the Java API
that is extensible for future proxy implementations or parameters to the proxy
connections itself without exposing the internals of the socket implementation.
By not having this conversation we have implemented something that solves
today's issue but will need to be deprecated or augmented to support future
changes to the proxy strategy.
----------------------------------------------------------------
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:
us...@infra.apache.org
