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 2021/11/05 11:13:35 UTC
[GitHub] [geode-native] gaussianrecurrence opened a new pull request #891: GEODE-9753: Solve PDX serialization coredump
gaussianrecurrence opened a new pull request #891:
URL: https://github.com/apache/geode-native/pull/891
- While serializing a PdxSerializable object, there is a possible race
condition which might cause the client to crash.
This race-condition happens whenever the cluster is restarted during
the serialization process and if
on-client-disconnect-clear-pdxType-Ids is set to true, meaning the
PdxTypeRegistry will be cleaned up if the connection towards the
cluster is lost.
- This issue has been solved by using the previously fetched local PDX
type.
- In order to properly test this solution, PdxRemoteWriterFactory has been added,
so the race-condition can be force at test-time.
- A new IT has been added to test that the solution is working fine.
- make_unique was needed inside cppcache/src, so it was moved to
utils/cxx_extensions.hpp
- Also in order to ease the use of newer C++ standard a preprocessor
check was added to make_unique, so if the used standard >= C++14, the
standard implementation of make_unique will be used instead.
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813186082
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -137,28 +136,18 @@ void PdxHelper::serializePdx(
// so we don't know whether user has used those or not;; Can we do some
// trick here?
- auto createPdxRemoteWriter = [&]() -> PdxRemoteWriter {
- if (auto pd = pdxTypeRegistry->getPreserveData(pdxObject)) {
- auto mergedPdxType = pdxTypeRegistry->getPdxType(pd->getMergedTypeId());
- return PdxRemoteWriter(output, mergedPdxType, pd, pdxTypeRegistry);
- } else {
- return PdxRemoteWriter(output, className, pdxTypeRegistry);
- }
- };
+ auto prw = cacheImpl->getPdxRemoteWriterFactory().create(
Review comment:
Exactly, I needed to force the race-condition and this was the only way I came up with. I am not sure there is any other way to force the race-condition.
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #891:
URL: https://github.com/apache/geode-native/pull/891#issuecomment-1048308540
> * This issue has been solved by using the previously fetched local PDX
> type.
Do you mean this issue is solved in this PR or in a previous and this PR just adds more testing?
--
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
[GitHub] [geode-native] gaussianrecurrence commented on pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #891:
URL: https://github.com/apache/geode-native/pull/891#issuecomment-1049080613
> > * This issue has been solved by using the previously fetched local PDX
> > type.
>
> Do you mean this issue is solved in this PR or in a previous and this PR just adds more testing?
The issue I am referring to here is the one I described in the bullet above.
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813199054
##########
File path: cppcache/src/PdxRemoteWriter.hpp
##########
@@ -22,11 +22,13 @@
#include "PdxLocalWriter.hpp"
+#include <geode/internal/geode_globals.hpp>
+
namespace apache {
namespace geode {
namespace client {
-class PdxRemoteWriter : public PdxLocalWriter {
+class APACHE_GEODE_EXPORT PdxRemoteWriter : public PdxLocalWriter {
Review comment:
This class is exported to allow testing. Thing is I thought there are no issues from the ABI-side if you added new symbols, rather than whenever you changed symbols. Am I wrong?
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813202990
##########
File path: cppcache/src/PdxRemoteWriterFactoryImpl.cpp
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "PdxRemoteWriterFactoryImpl.hpp"
+
+#include "PdxRemoteWriter.hpp"
+#include "PdxType.hpp"
+#include "PdxTypeRegistry.hpp"
+#include "util/cxx_extensions.hpp"
+
+namespace apache {
+namespace geode {
+namespace client {
+
+PdxRemoteWriterFactoryImpl::~PdxRemoteWriterFactoryImpl() = default;
+
+std::unique_ptr<PdxRemoteWriter> PdxRemoteWriterFactoryImpl::create(
Review comment:
Your approach seems promising. I'll look into it and see if it's feasible.
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813197629
##########
File path: cppcache/src/PdxRemoteWriter.cpp
##########
@@ -49,18 +49,18 @@ PdxRemoteWriter::PdxRemoteWriter(
}
PdxRemoteWriter::PdxRemoteWriter(
- DataOutput& output, std::string pdxClassName,
+ DataOutput& output, std::shared_ptr<PdxType> pdxType,
std::shared_ptr<PdxTypeRegistry> pdxTypeRegistry)
- : PdxLocalWriter(output, nullptr, pdxClassName, pdxTypeRegistry),
+ : PdxLocalWriter(output, pdxType, pdxTypeRegistry),
m_preserveDataIdx(0),
m_currentDataIdx(-1),
m_remoteTolocalMapLength(0) {
- m_preserveData = nullptr;
if (m_pdxType != nullptr) {
Review comment:
Thing is I did leave that because I thought what if pdxType passed were nullptr, but that's not the way it's intended to be used, so I guess it would be best to force a coredump if pdxType is nullptr, right?
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813186082
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -137,28 +136,18 @@ void PdxHelper::serializePdx(
// so we don't know whether user has used those or not;; Can we do some
// trick here?
- auto createPdxRemoteWriter = [&]() -> PdxRemoteWriter {
- if (auto pd = pdxTypeRegistry->getPreserveData(pdxObject)) {
- auto mergedPdxType = pdxTypeRegistry->getPdxType(pd->getMergedTypeId());
- return PdxRemoteWriter(output, mergedPdxType, pd, pdxTypeRegistry);
- } else {
- return PdxRemoteWriter(output, className, pdxTypeRegistry);
- }
- };
+ auto prw = cacheImpl->getPdxRemoteWriterFactory().create(
Review comment:
Indeed. I've seen you proposed an alternative way of testing this. I'll look into it.
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813204115
##########
File path: cppcache/src/PdxRemoteWriterFactoryImpl.hpp
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#ifndef GEODE_PDXREMOTEWRITERFACTORYIMPL_H_
+#define GEODE_PDXREMOTEWRITERFACTORYIMPL_H_
+
+#include "PdxRemoteWriterFactory.hpp"
+
+
+namespace apache {
+namespace geode {
+namespace client {
+
+class APACHE_GEODE_EXPORT PdxRemoteWriterFactoryImpl : public virtual PdxRemoteWriterFactory {
Review comment:
This is exported for testing purposes. Will try to test it the way @pivotal-jbarrett, which would avoid exporting symbols.
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r812447039
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -137,28 +136,18 @@ void PdxHelper::serializePdx(
// so we don't know whether user has used those or not;; Can we do some
// trick here?
- auto createPdxRemoteWriter = [&]() -> PdxRemoteWriter {
- if (auto pd = pdxTypeRegistry->getPreserveData(pdxObject)) {
- auto mergedPdxType = pdxTypeRegistry->getPdxType(pd->getMergedTypeId());
- return PdxRemoteWriter(output, mergedPdxType, pd, pdxTypeRegistry);
- } else {
- return PdxRemoteWriter(output, className, pdxTypeRegistry);
- }
- };
+ auto prw = cacheImpl->getPdxRemoteWriterFactory().create(
+ output, pdxObject, pdxTypeRegistry, localPdxType);
- PdxRemoteWriter prw = createPdxRemoteWriter();
-
- pdxObject->toData(prw);
- prw.endObjectWriting();
+ pdxObject->toData(*prw);
+ prw->endObjectWriting();
//[ToDo] need to write bytes for stats
- if (cacheImpl != nullptr) {
- uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +
- prw.getStartPositionOffset();
- int pdxLen = PdxHelper::readInt32(stPos);
- cachePerfStats.incPdxSerialization(
- pdxLen + 1 + 2 * 4); // pdxLen + 93 DSID + len + typeID
- }
+ uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +
Review comment:
Any idea why there was a `nullptr` check here?
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -121,13 +122,11 @@ void PdxHelper::serializePdx(
pdxTypeRegistry->addLocalPdxType(className, nType);
pdxTypeRegistry->addPdxType(nTypeId, nType);
- if (cacheImpl != nullptr) {
- uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +
- ptc.getStartPositionOffset();
- int pdxLen = PdxHelper::readInt32(stPos);
- cachePerfStats.incPdxSerialization(
- pdxLen + 1 + 2 * 4); // pdxLen + 93 DSID + len + typeID
- }
+ uint8_t* stPos =
Review comment:
`auto`
##########
File path: cppcache/src/PdxRemoteWriter.cpp
##########
@@ -49,18 +49,18 @@ PdxRemoteWriter::PdxRemoteWriter(
}
PdxRemoteWriter::PdxRemoteWriter(
- DataOutput& output, std::string pdxClassName,
+ DataOutput& output, std::shared_ptr<PdxType> pdxType,
std::shared_ptr<PdxTypeRegistry> pdxTypeRegistry)
- : PdxLocalWriter(output, nullptr, pdxClassName, pdxTypeRegistry),
+ : PdxLocalWriter(output, pdxType, pdxTypeRegistry),
m_preserveDataIdx(0),
m_currentDataIdx(-1),
m_remoteTolocalMapLength(0) {
- m_preserveData = nullptr;
if (m_pdxType != nullptr) {
- m_remoteTolocalMapLength = m_pdxType->getTotalFields();
m_remoteTolocalMap = m_pdxType->getRemoteToLocalMap();
+ m_remoteTolocalMapLength = m_pdxType->getTotalFields();
}
- initialize();
+
+ m_pdxClassName = pdxType->getPdxClassName();
Review comment:
If `pdxType` is nullable then this line will throw.
##########
File path: cppcache/src/PdxRemoteWriterFactoryImpl.cpp
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "PdxRemoteWriterFactoryImpl.hpp"
+
+#include "PdxRemoteWriter.hpp"
+#include "PdxType.hpp"
+#include "PdxTypeRegistry.hpp"
+#include "util/cxx_extensions.hpp"
+
+namespace apache {
+namespace geode {
+namespace client {
+
+PdxRemoteWriterFactoryImpl::~PdxRemoteWriterFactoryImpl() = default;
+
+std::unique_ptr<PdxRemoteWriter> PdxRemoteWriterFactoryImpl::create(
Review comment:
It really seems like you could test this at unit level by mocking `PdxTypeRegistery` and `PdxType`. Make this a member function back from where it came and test it a the unit level. This avoids the creation of this factory, the exporting of internal symbols, and the interdiction of artificial behavior into the production implementation, via overriding this implementation.
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -137,28 +136,18 @@ void PdxHelper::serializePdx(
// so we don't know whether user has used those or not;; Can we do some
// trick here?
- auto createPdxRemoteWriter = [&]() -> PdxRemoteWriter {
- if (auto pd = pdxTypeRegistry->getPreserveData(pdxObject)) {
- auto mergedPdxType = pdxTypeRegistry->getPdxType(pd->getMergedTypeId());
- return PdxRemoteWriter(output, mergedPdxType, pd, pdxTypeRegistry);
- } else {
- return PdxRemoteWriter(output, className, pdxTypeRegistry);
- }
- };
+ auto prw = cacheImpl->getPdxRemoteWriterFactory().create(
+ output, pdxObject, pdxTypeRegistry, localPdxType);
- PdxRemoteWriter prw = createPdxRemoteWriter();
-
- pdxObject->toData(prw);
- prw.endObjectWriting();
+ pdxObject->toData(*prw);
+ prw->endObjectWriting();
//[ToDo] need to write bytes for stats
- if (cacheImpl != nullptr) {
- uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +
- prw.getStartPositionOffset();
- int pdxLen = PdxHelper::readInt32(stPos);
- cachePerfStats.incPdxSerialization(
- pdxLen + 1 + 2 * 4); // pdxLen + 93 DSID + len + typeID
- }
+ uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +
Review comment:
`auto`
##########
File path: cppcache/src/PdxRemoteWriter.cpp
##########
@@ -49,18 +49,18 @@ PdxRemoteWriter::PdxRemoteWriter(
}
PdxRemoteWriter::PdxRemoteWriter(
- DataOutput& output, std::string pdxClassName,
+ DataOutput& output, std::shared_ptr<PdxType> pdxType,
std::shared_ptr<PdxTypeRegistry> pdxTypeRegistry)
- : PdxLocalWriter(output, nullptr, pdxClassName, pdxTypeRegistry),
+ : PdxLocalWriter(output, pdxType, pdxTypeRegistry),
m_preserveDataIdx(0),
m_currentDataIdx(-1),
m_remoteTolocalMapLength(0) {
- m_preserveData = nullptr;
if (m_pdxType != nullptr) {
Review comment:
If `pdxType` is not nullable then this check is unnecessary.
##########
File path: cppcache/src/PdxRemoteWriter.hpp
##########
@@ -45,10 +47,10 @@ class PdxRemoteWriter : public PdxLocalWriter {
PdxRemoteWriter() = default;
PdxRemoteWriter(DataOutput& output, std::shared_ptr<PdxType> pdxType,
- std::shared_ptr<PdxRemotePreservedData> preservedData,
std::shared_ptr<PdxTypeRegistry> pdxTypeRegistry);
- PdxRemoteWriter(DataOutput& output, std::string pdxClassName,
+ PdxRemoteWriter(DataOutput& output, std::shared_ptr<PdxType> pdxType,
+ std::shared_ptr<PdxRemotePreservedData> preservedData,
Review comment:
It this really feels like an extension of the other constructor with the optional `preservedData` parameters to maybe re-order the parameters to look like that.
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #891:
URL: https://github.com/apache/geode-native/pull/891#issuecomment-1048315939
> > * This issue has been solved by using the previously fetched local PDX
> > type.
>
> Do you mean this issue is solved in this PR or in a previous and this PR just adds more testing?
Ok, I think I found the fix but I a not sure I understand it. How does preserving the `PdxType` in all cases of the `RemotePdxWriter` construction fix the race condition?
--
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
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r812415266
##########
File path: cppcache/src/PdxRemoteWriterFactoryImpl.hpp
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#ifndef GEODE_PDXREMOTEWRITERFACTORYIMPL_H_
+#define GEODE_PDXREMOTEWRITERFACTORYIMPL_H_
+
+#include "PdxRemoteWriterFactory.hpp"
+
+
+namespace apache {
+namespace geode {
+namespace client {
+
+class APACHE_GEODE_EXPORT PdxRemoteWriterFactoryImpl : public virtual PdxRemoteWriterFactory {
Review comment:
Again, please don't export.
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -137,28 +136,18 @@ void PdxHelper::serializePdx(
// so we don't know whether user has used those or not;; Can we do some
// trick here?
- auto createPdxRemoteWriter = [&]() -> PdxRemoteWriter {
- if (auto pd = pdxTypeRegistry->getPreserveData(pdxObject)) {
- auto mergedPdxType = pdxTypeRegistry->getPdxType(pd->getMergedTypeId());
- return PdxRemoteWriter(output, mergedPdxType, pd, pdxTypeRegistry);
- } else {
- return PdxRemoteWriter(output, className, pdxTypeRegistry);
- }
- };
+ auto prw = cacheImpl->getPdxRemoteWriterFactory().create(
Review comment:
Can we use a slightly longer, meaningful name? like `remoteWriter` for instance?
##########
File path: cppcache/src/PdxRemoteWriter.hpp
##########
@@ -22,11 +22,13 @@
#include "PdxLocalWriter.hpp"
+#include <geode/internal/geode_globals.hpp>
+
namespace apache {
namespace geode {
namespace client {
-class PdxRemoteWriter : public PdxLocalWriter {
+class APACHE_GEODE_EXPORT PdxRemoteWriter : public PdxLocalWriter {
Review comment:
Please don't export this, we can't change the ABI and anyway we don't want any more publicly-accessible things than we already have(!).
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813196087
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -137,28 +136,18 @@ void PdxHelper::serializePdx(
// so we don't know whether user has used those or not;; Can we do some
// trick here?
- auto createPdxRemoteWriter = [&]() -> PdxRemoteWriter {
- if (auto pd = pdxTypeRegistry->getPreserveData(pdxObject)) {
- auto mergedPdxType = pdxTypeRegistry->getPdxType(pd->getMergedTypeId());
- return PdxRemoteWriter(output, mergedPdxType, pd, pdxTypeRegistry);
- } else {
- return PdxRemoteWriter(output, className, pdxTypeRegistry);
- }
- };
+ auto prw = cacheImpl->getPdxRemoteWriterFactory().create(
+ output, pdxObject, pdxTypeRegistry, localPdxType);
- PdxRemoteWriter prw = createPdxRemoteWriter();
-
- pdxObject->toData(prw);
- prw.endObjectWriting();
+ pdxObject->toData(*prw);
+ prw->endObjectWriting();
//[ToDo] need to write bytes for stats
- if (cacheImpl != nullptr) {
- uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +
- prw.getStartPositionOffset();
- int pdxLen = PdxHelper::readInt32(stPos);
- cachePerfStats.incPdxSerialization(
- pdxLen + 1 + 2 * 4); // pdxLen + 93 DSID + len + typeID
- }
+ uint8_t* stPos = const_cast<uint8_t*>(output.getBuffer()) +
Review comment:
Well, not really, and it doesn't make any sense, since CacheImpl should exist as long as there is a cache and even if that block of code should not be executed whenever the cache is closed, given cacheImpl is a pointer, the code would still execute. This would only would make sense in the case that cacheImpl were a std::weak_ptr<CacheImpl> instead, but that's not the case.
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813199054
##########
File path: cppcache/src/PdxRemoteWriter.hpp
##########
@@ -22,11 +22,13 @@
#include "PdxLocalWriter.hpp"
+#include <geode/internal/geode_globals.hpp>
+
namespace apache {
namespace geode {
namespace client {
-class PdxRemoteWriter : public PdxLocalWriter {
+class APACHE_GEODE_EXPORT PdxRemoteWriter : public PdxLocalWriter {
Review comment:
This is exported for testing purposes. Will try to test it the way @pivotal-jbarrett, which would avoid exporting symbols.
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r812444100
##########
File path: cppcache/src/PdxHelper.cpp
##########
@@ -137,28 +136,18 @@ void PdxHelper::serializePdx(
// so we don't know whether user has used those or not;; Can we do some
// trick here?
- auto createPdxRemoteWriter = [&]() -> PdxRemoteWriter {
- if (auto pd = pdxTypeRegistry->getPreserveData(pdxObject)) {
- auto mergedPdxType = pdxTypeRegistry->getPdxType(pd->getMergedTypeId());
- return PdxRemoteWriter(output, mergedPdxType, pd, pdxTypeRegistry);
- } else {
- return PdxRemoteWriter(output, className, pdxTypeRegistry);
- }
- };
+ auto prw = cacheImpl->getPdxRemoteWriterFactory().create(
Review comment:
What this extracted into an pure virtual interface for the sole purpose of integration testing?
--
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
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #891:
URL: https://github.com/apache/geode-native/pull/891#discussion_r813230536
##########
File path: cppcache/src/PdxRemoteWriter.cpp
##########
@@ -49,18 +49,18 @@ PdxRemoteWriter::PdxRemoteWriter(
}
PdxRemoteWriter::PdxRemoteWriter(
- DataOutput& output, std::string pdxClassName,
+ DataOutput& output, std::shared_ptr<PdxType> pdxType,
std::shared_ptr<PdxTypeRegistry> pdxTypeRegistry)
- : PdxLocalWriter(output, nullptr, pdxClassName, pdxTypeRegistry),
+ : PdxLocalWriter(output, pdxType, pdxTypeRegistry),
m_preserveDataIdx(0),
m_currentDataIdx(-1),
m_remoteTolocalMapLength(0) {
- m_preserveData = nullptr;
if (m_pdxType != nullptr) {
Review comment:
Yeah, if it is `nullptr` then line 63 is going to fault anyway. Also, if passed here as `nulltpr` but later use excepted `m_pdxType` to not be `nullptr`... boom.
--
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
[GitHub] [geode-native] gaussianrecurrence commented on pull request #891: GEODE-9753: Solve PDX serialization coredump
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #891:
URL: https://github.com/apache/geode-native/pull/891#issuecomment-1011957005
I am reviewing all of my pending PRs, and this has been awating review for quite some time. Maybe @pdxcodemonkey, @pivotal-jbarrett @albertogpz @mkevo @jvarenina or anyone else could take a look at it and provide some feedback?
Thanks!
--
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