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/06 16:50:11 UTC
[GitHub] [geode-native] mmartell opened a new pull request, #954: GEODE-10218: Allow server debug
mmartell opened a new pull request, #954:
URL: https://github.com/apache/geode-native/pull/954
To assist with protocol analysis it is very helpful to be able to step through the server code in the Intellij debugger while running native client test code. However, to be allowed to set breakpoints in the server code, the server must be started with the special flag below:
```
--J=-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=127.0.0.1:FREEPORT```
This ticket is to add support for the above flag in the native client test frameworks.
--
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 diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r869645866
##########
clicache/integration-test2/GfshTest.cs:
##########
@@ -77,13 +77,16 @@ public void StartLocatorStringsTest()
.withSslKeyStore("some/path/keystore.jks")
.withSslKeyStorePassword("password1")
.withSslTrustStore("some/path/truststore.jks")
- .withSslTrustStorePassword("password2");
+ .withSslTrustStorePassword("password2")
+ .withDebugAgent(true, "address");
s = locator.ToString();
+ var withAttachPortRemoved = s.Substring(0, s.LastIndexOf(":", s.Length-1, 6));
Review Comment:
"attachPort"?
##########
cppcache/integration/framework/Cluster.h:
##########
@@ -141,6 +142,9 @@ class Cluster {
Cluster(InitialLocators initialLocators, InitialServers initialServers,
UseIpv6 useIpv6 = UseIpv6{false});
+ Cluster(InitialLocators initialLocators, InitialServers initialServers,
+ UseDebugAgent allowAttach);
Review Comment:
rename variable.
##########
cppcache/integration/framework/Cluster.h:
##########
@@ -124,6 +124,7 @@ using Password = NamedType<std::string, struct PasswordParameter>;
using CacheXMLFiles =
NamedType<std::vector<std::string>, struct CacheXMLFilesParameter>;
using UseIpv6 = NamedType<bool, struct UseIpv6Parameter>;
+using UseDebugAgent = NamedType<bool, struct AllowAttachParameter>;
Review Comment:
Rename struct.
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r869733477
##########
cppcache/integration/framework/Cluster.h:
##########
@@ -141,6 +142,9 @@ class Cluster {
Cluster(InitialLocators initialLocators, InitialServers initialServers,
UseIpv6 useIpv6 = UseIpv6{false});
+ Cluster(InitialLocators initialLocators, InitialServers initialServers,
+ UseDebugAgent allowAttach);
Review Comment:
Done
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r869734411
##########
cppcache/integration/framework/Cluster.h:
##########
@@ -124,6 +124,7 @@ using Password = NamedType<std::string, struct PasswordParameter>;
using CacheXMLFiles =
NamedType<std::vector<std::string>, struct CacheXMLFilesParameter>;
using UseIpv6 = NamedType<bool, struct UseIpv6Parameter>;
+using UseDebugAgent = NamedType<bool, struct AllowAttachParameter>;
Review Comment:
Done
--
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] lgtm-com[bot] commented on pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on PR #954:
URL: https://github.com/apache/geode-native/pull/954#issuecomment-1123172546
This pull request **introduces 1 alert** when merging 927e176d7433e029d7cae2ca7edd64d658f0ceac into 4d0578a829ee4d967ec4358133e4a8c3563f1310 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-f563aca93e0096a331c3a653777f131820e36a0d)
**new alerts:**
* 1 for Useless assignment to local variable
--
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] mmartell merged pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell merged PR #954:
URL: https://github.com/apache/geode-native/pull/954
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r846162809
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -71,7 +73,7 @@ private bool StartLocators()
for (var i = 0; i < locatorCount_; i++)
{
var locator = new Locator(this, new List<Locator>(),
Review Comment:
Good catch!
--
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] lgtm-com[bot] commented on pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on PR #954:
URL: https://github.com/apache/geode-native/pull/954#issuecomment-1123054711
This pull request **introduces 1 alert** when merging 4329b35950257a683e7654abce5ef4f91cb3c609 into 4d0578a829ee4d967ec4358133e4a8c3563f1310 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-2cc182192ac72cdaa2e11c915a8d06927a438ed1)
**new alerts:**
* 1 for Useless assignment to local variable
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r869537870
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -71,7 +73,7 @@ private bool StartLocators()
for (var i = 0; i < locatorCount_; i++)
{
var locator = new Locator(this, new List<Locator>(),
- name_ + "/locator/" + i.ToString(), i == 0);
+ name_ + "/locator/" + i.ToString(), i == 0, allowAttach_);
Review Comment:
Done.
--
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 diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r846126196
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -71,7 +73,7 @@ private bool StartLocators()
for (var i = 0; i < locatorCount_; i++)
{
var locator = new Locator(this, new List<Locator>(),
- name_ + "/locator/" + i.ToString(), i == 0);
+ name_ + "/locator/" + i.ToString(), i == 0, allowAttach_);
Review Comment:
This constructor is starting to get wild. What about a builder patter?
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -200,6 +204,7 @@ public int Start()
.withMaxHeap("256m")
.withJmxManagerPort(cluster_.jmxManagerPort)
.withJmxManagerStart(startJmxManager_)
+ .withAllowAttach(allowAttach_, Address.address)
Review Comment:
What is behind the choice of the word "attach"? We are starting the debugger agent, why not use a name that suggests that, like `withDebugAgent(...)`.
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -71,7 +73,7 @@ private bool StartLocators()
for (var i = 0; i < locatorCount_; i++)
{
var locator = new Locator(this, new List<Locator>(),
Review Comment:
The second parameter here should be your `locators_` list so that each locators is aware of the previously created.
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r846170170
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -71,7 +73,7 @@ private bool StartLocators()
for (var i = 0; i < locatorCount_; i++)
{
var locator = new Locator(this, new List<Locator>(),
- name_ + "/locator/" + i.ToString(), i == 0);
+ name_ + "/locator/" + i.ToString(), i == 0, allowAttach_);
Review Comment:
I do like the builder pattern. With nearly 30 potential parameters for the start locator command it seems prudent. Means using it for Server construction also, for consistency.
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r846170170
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -71,7 +73,7 @@ private bool StartLocators()
for (var i = 0; i < locatorCount_; i++)
{
var locator = new Locator(this, new List<Locator>(),
- name_ + "/locator/" + i.ToString(), i == 0);
+ name_ + "/locator/" + i.ToString(), i == 0, allowAttach_);
Review Comment:
I do like the builder pattern. With nearly 30 potential parameters for the start locator command it seems prudent.
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r846172625
##########
clicache/integration-test2/Cluster.cs:
##########
@@ -200,6 +204,7 @@ public int Start()
.withMaxHeap("256m")
.withJmxManagerPort(cluster_.jmxManagerPort)
.withJmxManagerStart(startJmxManager_)
+ .withAllowAttach(allowAttach_, Address.address)
Review Comment:
I like it! Good rename.
--
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] mmartell commented on a diff in pull request #954: GEODE-10218: Allow server debug
Posted by GitBox <gi...@apache.org>.
mmartell commented on code in PR #954:
URL: https://github.com/apache/geode-native/pull/954#discussion_r869732762
##########
clicache/integration-test2/GfshTest.cs:
##########
@@ -77,13 +77,16 @@ public void StartLocatorStringsTest()
.withSslKeyStore("some/path/keystore.jks")
.withSslKeyStorePassword("password1")
.withSslTrustStore("some/path/truststore.jks")
- .withSslTrustStorePassword("password2");
+ .withSslTrustStorePassword("password2")
+ .withDebugAgent(true, "address");
s = locator.ToString();
+ var withAttachPortRemoved = s.Substring(0, s.LastIndexOf(":", s.Length-1, 6));
Review Comment:
Good catch. No longer using attach nomenclature.
--
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