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/04/30 20:58:54 UTC
[GitHub] [geode-native] pdxcodemonkey opened a new pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
pdxcodemonkey opened a new pull request #800:
URL: https://github.com/apache/geode-native/pull/800
Sending GET_CLIENT_PR_METADATA to a server for a replicated region generated an EXCEPTION response message, which the native client would then ignore. Convention in the Java client is to check for a bucket count of -1, indicating replicated region, and skip the metadata request in that case.
@gaussianrecurrence
--
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:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #800:
URL: https://github.com/apache/geode-native/pull/800#issuecomment-832003167
> > Sending GET_CLIENT_PR_METADATA to a server for a replicated region generated an EXCEPTION response message, which the native client would then ignore. Convention in the Java client is to check for a bucket count of -1, indicating replicated region, and skip the metadata request in that case.
> > @gaussianrecurrence
>
> > I wonder if it's best to add this behaviour to enqueueForMetadataRefresh instead. Maybe that function could be refactored, so instead of accepting the full path, it accepts the region object. This way it can be check if the region is partitioned or replicated. And if replicated, then don't add the request to the queue.
>
> Given the complexity of our dependency graph, I'm reluctant to pass around yet another reference to a `region` object that must be accounted for at shutdown, esp to a thread with dubious timing like the metadata service. We could, however, potentially save some network traffic by storing a map of region name to server region type in `ClientMetadataService` _if_ we knew it was not possible for server region type to change "on the fly". @pivotal-jbarrett do you know if this is the case? If so, we could save the server region type and never send GET_CLIENT_PARTITION_ATTRIBUTES for a known region with replicated type. I also wonder if the optimization is worth the effort, though, since metadata refresh isn't intended to be a high-frequency event.
Sorry for the confusion, I thought you had the information about whether a region is partitioned or replicated just after you registered the region in the client.
And as for the optimization you pointed out, as you said, metadata update should only happen upon rebalance, which is not a pretty common event, so I'd say there's really no save there.
--
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:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #800:
URL: https://github.com/apache/geode-native/pull/800#issuecomment-832121007
@pivotal-jbarrett Filed this ticket to follow up: https://issues.apache.org/jira/browse/GEODE-9234
--
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:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #800:
URL: https://github.com/apache/geode-native/pull/800
--
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:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #800:
URL: https://github.com/apache/geode-native/pull/800#issuecomment-830420598
I wonder if it's best to add this behaviour to enqueueForMetadataRefresh instead. Maybe that function could be refactored, so instead of accepting the full path, it accepts the region object. This way it can be check if the region is partitioned or replicated. And if replicated, then don't add the request to the queue.
--
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:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #800:
URL: https://github.com/apache/geode-native/pull/800#issuecomment-832003876
I don’t think there is any way for the region type to change on the server without the region being torn down and recreated.
In the ClientMetadataService::enqueueForMetadataRefresh method it looks up the Region already. It doesn’t keep a reference to it. If ThinClientRegion has type enumeration or bucket count we can do the check there and ignore the enqueue the name. No need to enqueue the reference.
For extra credit there are some ugly bugs in this code.
On May 4, 2021, at 7:24 AM, Blake Bender ***@***.******@***.***>> wrote:
Sending GET_CLIENT_PR_METADATA to a server for a replicated region generated an EXCEPTION response message, which the native client would then ignore. Convention in the Java client is to check for a bucket count of -1, indicating replicated region, and skip the metadata request in that case.
@gaussianrecurrence<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgaussianrecurrence&data=04%7C01%7Cjabarrett%40vmware.com%7Cc0f1071a318e44dd4fe308d90f085a92%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637557350831814485%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=OFHeGCttJY1H%2FzDS%2FIWi8VOdQjNzmx38bjKRxo%2FxMMY%3D&reserved=0>
I wonder if it's best to add this behaviour to enqueueForMetadataRefresh instead. Maybe that function could be refactored, so instead of accepting the full path, it accepts the region object. This way it can be check if the region is partitioned or replicated. And if replicated, then don't add the request to the queue.
Given the complexity of our dependency graph, I'm reluctant to pass around yet another reference to a region object that must be accounted for at shutdown, esp to a thread with dubious timing like the metadata service. We could, however, potentially save some network traffic by storing a map of region name to server region type in ClientMetadataService if we knew it was not possible for server region type to change "on the fly". @pivotal-jbarrett<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpivotal-jbarrett&data=04%7C01%7Cjabarrett%40vmware.com%7Cc0f1071a318e44dd4fe308d90f085a92%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637557350831824478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=84l7Qqp200eDrs2R%2FInN16L5YAz2F0Tdd%2FNlFCPR6k4%3D&reserved=0> do you know if this is the case? If so, we could save the server region type and never send GET_CLIENT_PARTITION_ATTRIBUTES for a known region w
ith replicated type. I also wonder if the optimization is worth the effort, though, since metadata refresh isn't intended to be a high-frequency event.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode-native%2Fpull%2F800%23issuecomment-831982865&data=04%7C01%7Cjabarrett%40vmware.com%7Cc0f1071a318e44dd4fe308d90f085a92%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637557350831834473%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=hQbH4tX7ix7V00VnEc1Q9Leejr5UikPabM44J0ZP2pA%3D&reserved=0>, or unsubscribe<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABT4LF725BDPCF4WS2UPQNTTL77SRANCNFSM435O3ZJQ&data=04%7C01%7Cjabarrett%40vmware.com%7Cc0f1071a318e44dd4fe308d90f085a92%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637557350831834473%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ru2LPIR9XorxW0JwCDxbhQbg85JsQTTJyzSbL6LRW9Q%3D&reserved=0>.
--
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:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #800:
URL: https://github.com/apache/geode-native/pull/800#discussion_r625129556
##########
File path: cppcache/src/ClientMetadataService.cpp
##########
@@ -121,6 +121,14 @@ void ClientMetadataService::getClientPRMetadata(const char* regionFullPath) {
if (err == GF_NOERR &&
reply.getMessageType() ==
TcrMessage::RESPONSE_CLIENT_PARTITION_ATTRIBUTES) {
+ // By convention, server returns -1 bucket count to indicate replicated
+ // region
+ if (reply.getNumBuckets() == -1) {
+ LOGDEBUG(
Review comment:
I am not sure this is worth logging. Knowing this information is unlikely to help us debug anything.
--
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:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #800: GEODE-8405: Don't ask for metadata for replicated regions
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #800:
URL: https://github.com/apache/geode-native/pull/800#issuecomment-831982865
> Sending GET_CLIENT_PR_METADATA to a server for a replicated region generated an EXCEPTION response message, which the native client would then ignore. Convention in the Java client is to check for a bucket count of -1, indicating replicated region, and skip the metadata request in that case.
>
> @gaussianrecurrence
> I wonder if it's best to add this behaviour to enqueueForMetadataRefresh instead. Maybe that function could be refactored, so instead of accepting the full path, it accepts the region object. This way it can be check if the region is partitioned or replicated. And if replicated, then don't add the request to the queue.
Given the complexity of our dependency graph, I'm reluctant to pass around yet another reference to a `region` object that must be accounted for at shutdown, esp to a thread with dubious timing like the metadata service. We could, however, potentially save some network traffic by storing a map of region name to server region type in `ClientMetadataService` *if* we knew it was not possible for server region type to change "on the fly". @pivotal-jbarrett do you know if this is the case? If so, we could save the server region type and never send GET_CLIENT_PARTITION_ATTRIBUTES for a known region with replicated type. I also wonder if the optimization is worth the effort, though, since metadata refresh isn't intended to be a high-frequency event.
--
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:
users@infra.apache.org