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 2022/04/28 17:56:30 UTC
[GitHub] [geode-native] pdxcodemonkey commented on a diff in pull request #962: GEODE-10259: Update protocol ordinal to 1.14.0 (125)
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?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org