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