[ https://issues.apache.org/jira/browse/GEODE-10259?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17529564#comment-17529564 ]
ASF GitHub Bot commented on GEODE-10259: ---------------------------------------- pdxcodemonkey commented on code in PR #962: URL: https://github.com/apache/geode-native/pull/962#discussion_r861014615 ########## cppcache/integration/test/PdxJsonTypeTest.cpp: ########## @@ -101,8 +101,7 @@ TEST(PdxJsonTypeTest, testGfshQueryJsonInstances) { cache.createPdxInstanceFactory(gemfireJsonClassName) .writeString("entryName", "java-domain-class-entry") .create()); - ASSERT_THROW(execution.withArgs(query).execute("QueryFunction"), - CacheServerException); + ASSERT_NO_THROW(execution.withArgs(query).execute("QueryFunction")); Review Comment: Wait, wut? Why does this stop throwing? ########## cppcache/src/ClientHealthStats.cpp: ########## @@ -14,62 +14,58 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + #include "ClientHealthStats.hpp" -#include "CacheImpl.hpp" +#include <geode/CacheableDate.hpp> +#include <geode/DataInput.hpp> +#include <geode/DataOutput.hpp> namespace apache { namespace geode { namespace client { void ClientHealthStats::toData(DataOutput& output) const { - output.writeInt(static_cast<int32_t>(m_numGets)); - output.writeInt(static_cast<int32_t>(m_numPuts)); - output.writeInt(static_cast<int32_t>(m_numMisses)); - output.writeInt(static_cast<int32_t>(m_numCacheListenerCalls)); - output.writeInt(static_cast<int32_t>(m_numThread)); - output.writeInt(static_cast<int32_t>(m_cpus)); - output.writeInt(static_cast<int64_t>(m_processCpuTime)); - m_updateTime->toData(output); + output.writeInt(static_cast<int64_t>(gets_)); + output.writeInt(static_cast<int64_t>(puts_)); + output.writeInt(static_cast<int64_t>(misses_)); + output.writeInt(static_cast<int32_t>(cacheListenerCallsCompleted_)); + output.writeInt(static_cast<int32_t>(threads_)); + output.writeInt(static_cast<int32_t>(cpus_)); + output.writeInt(static_cast<int64_t>(processCpuTime_)); + updateTime_->toData(output); Review Comment: Did the size of the fields here change with the version of the protocol/server? Or has this just been wrong forever in the native client? If it changed, is there a way to determine which version of a stat file you have? ########## cppcache/src/ClientProxyMembershipID.cpp: ########## @@ -247,31 +243,26 @@ Serializable* ClientProxyMembershipID::readEssentialData(DataInput& input) { auto dsName = std::dynamic_pointer_cast<CacheableString>(input.readObject()); - initHostAddressVector(hostAddress, length); + initHostAddressVector(hostAddress.data(), length); - if (vmKind != ClientProxyMembershipID::LONER_DM_TYPE) { + if (vmKind != kVmKindLoaner) { // initialize the object with the values read and some dummy values - initObjectVars("", hostPort, "", std::chrono::seconds::zero(), DCPORT, 0, + initObjectVars("", hostPort, "", std::chrono::seconds::zero(), kDcPort, 0, vmKind, 0, dsName->value().c_str(), nullptr, vmViewId); } else { // initialize the object with the values read and some dummy values - initObjectVars("", hostPort, "", std::chrono::seconds::zero(), DCPORT, 0, + initObjectVars("", hostPort, "", std::chrono::seconds::zero(), kDcPort, 0, vmKind, 0, dsName->value().c_str(), uniqueTag->value().c_str(), vmViewId); } - - delete[] hostAddress; - readAdditionalData(input); - - return this; } void ClientProxyMembershipID::readAdditionalData(DataInput& input) { // Skip unused UUID (16) and weight (0); input.advanceCursor(17); } -void ClientProxyMembershipID::increaseSynchCounter() { ++synch_counter; } +void ClientProxyMembershipID::increaseSynchCounter() { ++synchCounter; } Review Comment: Can we please lose the 'h'? 'synch' isn't a normal English abbreviation for anything... ########## cppcache/src/ClientProxyMembershipID.cpp: ########## @@ -185,31 +182,31 @@ void ClientProxyMembershipID::toData(DataOutput&) const { void ClientProxyMembershipID::fromData(DataInput& input) { // deserialization for PR FX HA - auto length = input.readArrayLength(); - auto hostAddress = new uint8_t[length]; - input.readBytesOnly(hostAddress, length); - auto hostPort = input.readInt32(); - auto hostname = + const auto length = input.readArrayLength(); + auto hostAddress = std::vector<uint8_t>(length); + input.readBytesOnly(hostAddress.data(), length); + const auto hostPort = input.readInt32(); + const auto hostname = std::dynamic_pointer_cast<CacheableString>(input.readObject()); - auto splitbrain = input.read(); - auto dcport = input.readInt32(); - auto vPID = input.readInt32(); - auto vmKind = input.read(); - auto aStringArray = CacheableStringArray::create(); + const auto splitbrain = input.read(); + const auto dcport = input.readInt32(); + const auto vPID = input.readInt32(); + const auto vmKind = input.read(); + const auto aStringArray = CacheableStringArray::create(); aStringArray->fromData(input); - auto dsName = std::dynamic_pointer_cast<CacheableString>(input.readObject()); - auto uniqueTag = + const auto dsName = std::dynamic_pointer_cast<CacheableString>(input.readObject()); - auto durableClientId = + const auto uniqueTag = std::dynamic_pointer_cast<CacheableString>(input.readObject()); - auto durableClientTimeOut = std::chrono::seconds(input.readInt32()); - int32_t vmViewId = 0; + const auto durableClientId = + std::dynamic_pointer_cast<CacheableString>(input.readObject()); + const auto durableClientTimeOut = std::chrono::seconds(input.readInt32()); readVersion(splitbrain, input); - initHostAddressVector(hostAddress, length); + initHostAddressVector(hostAddress.data(), length); - if (vmKind != ClientProxyMembershipID::LONER_DM_TYPE) { - vmViewId = std::stoi(uniqueTag->value()); + if (vmKind != kVmKindLoaner) { + auto vmViewId = std::stoi(uniqueTag->value()); initObjectVars(hostname->value().c_str(), hostPort, durableClientId->value().c_str(), durableClientTimeOut, dcport, vPID, vmKind, splitbrain, dsName->value().c_str(), Review Comment: initObjectVars is a really awful generic name for a pretty awful function. Surely out of scope for this change, but it should probably be broken down into specific functions with meaningful names. ########## cppcache/src/ClientHealthStats.hpp: ########## @@ -20,57 +20,54 @@ #ifndef GEODE_CLIENTHEALTHSTATS_H_ #define GEODE_CLIENTHEALTHSTATS_H_ -#include <geode/CacheableDate.hpp> -#include <geode/Serializable.hpp> #include <geode/internal/DataSerializableFixedId.hpp> -#include "util/Log.hpp" - namespace apache { namespace geode { namespace client { +class CacheableDate; + class ClientHealthStats : public internal::DataSerializableFixedId_t< internal::DSFid::ClientHealthStats> { public: void toData(DataOutput& output) const override; void fromData(DataInput& input) override; - /** - * @brief creation function for dates. - */ static std::shared_ptr<Serializable> createDeserializable(); - /** @return the size of the object in bytes */ size_t objectSize() const override { return sizeof(ClientHealthStats); } + /** * Factory method for creating an instance of ClientHealthStats */ - static std::shared_ptr<ClientHealthStats> create(int gets, int puts, - int misses, int listCalls, - int numThreads, - int64_t cpuTime = 0, - int cpus = 0) { - return std::shared_ptr<ClientHealthStats>(new ClientHealthStats( - gets, puts, misses, listCalls, numThreads, cpuTime, cpus)); + static std::shared_ptr<ClientHealthStats> create( + int64_t gets, int64_t puts, int64_t misses, + int32_t cacheListenerCallsCompleted, int32_t threads, + int64_t processCpuTime = 0, int32_t cpus = 0) { + return std::make_shared<ClientHealthStats>(gets, puts, misses, + cacheListenerCallsCompleted, + threads, processCpuTime, cpus); } - ~ClientHealthStats() override = default; + + ~ClientHealthStats() noexcept override = default; ClientHealthStats(); + ClientHealthStats(int64_t gets, int64_t puts, int64_t misses, + int32_t cacheListenerCallsCompleted, int32_t threads, + int64_t processCpuTime, int32_t cpus); + private: - ClientHealthStats(int gets, int puts, int misses, int listCalls, - int numThreads, int64_t cpuTime, int cpus); - - int m_numGets; // CachePerfStats.gets - int m_numPuts; // CachePerfStats.puts - int m_numMisses; // CachePerfStats.misses - int m_numCacheListenerCalls; // CachePerfStats.cacheListenerCallsCompleted - int m_numThread; // ProcessStats.threads; - int64_t m_processCpuTime; // - int m_cpus; - std::shared_ptr<CacheableDate> m_updateTime; // Last updateTime + int64_t gets_; Review Comment: Thanks for renaming these. Hopefully we can completely stamp out 'm_' throughout the code base soon. ########## cppcache/src/ClientProxyMembershipID.cpp: ########## @@ -31,19 +26,20 @@ #include "Version.hpp" #include "util/Log.hpp" -#define DCPORT 12334 -#define VMKIND 13 -#define ROLEARRLENGTH 0 +namespace { +constexpr int32_t kVersionMask = 0x8; +constexpr int8_t kVmKindLoaner = 13; Review Comment: This appears to be a bad name, maybe just a typo? "loaner" means borrowed, prior constant name below contains "LONER", meaning by itself - not the same thing. Also why do we have the `kVmKind` alias for this constant? Looks like we probably only need one variable. ########## cppcache/src/ClientProxyMembershipID.hpp: ########## @@ -36,10 +35,6 @@ namespace apache { namespace geode { namespace client { -using internal::DSFid; - -class ClientProxyMembershipID; Review Comment: lolwut??? ########## cppcache/src/ClientProxyMembershipID.hpp: ########## @@ -51,26 +46,37 @@ class ClientProxyMembershipID : public DSMemberForVersionStamp { const std::chrono::seconds durableClientTimeOut = std::chrono::seconds::zero()); - // This constructor is only for testing and should not be used for any - // other purpose. See testEntriesMapForVersioning.cpp for more details + /** + * This constructor is only for testing and should not be used for any + * other purpose. See testEntriesMapForVersioning.cpp for more details + */ ClientProxyMembershipID(const uint8_t* hostAddr, uint32_t hostAddrLen, uint32_t hostPort, const char* dsname, const char* uniqueTag, uint32_t vmViewId); - // ClientProxyMembershipID(const char *durableClientId = nullptr, const - // uint32_t durableClntTimeOut = 0); + ClientProxyMembershipID(); + ~ClientProxyMembershipID() noexcept override; + static void increaseSynchCounter(); + static std::shared_ptr<Serializable> createDeserializable() { return std::make_shared<ClientProxyMembershipID>(); } - // Do an empty check on the returned value. Only use after handshake is done. + + /** + * Do an empty check on the returned value. Only use after handshake is done. + */ Review Comment: I'm not sure this comment adds any value. Also the name of this function doesn't appear to describe it correctly - it returns member var `clientId_`, maybe just call it getClientId? > Update geode-native library protocol to 1.14 > -------------------------------------------- > > Key: GEODE-10259 > URL: https://issues.apache.org/jira/browse/GEODE-10259 > Project: Geode > Issue Type: Improvement > Reporter: Jacob Barrett > Priority: Major > > The geode-native library still talks the Geode 1.0.0 protocol, update to at > 1.14. -- This message was sent by Atlassian Jira (v8.20.7#820007)