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 2020/10/14 13:52:00 UTC
[GitHub] [geode-native] mmartell opened a new pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
mmartell opened a new pull request #674:
URL: https://github.com/apache/geode-native/pull/674
This adds a new DropProxy test to the docker based acceptance tests for .NET.
Note: This test will only run on Windows platforms that have docker and docker-compose installed.
----------------------------------------------------------------
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] lgtm-com[bot] commented on pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #674:
URL: https://github.com/apache/geode-native/pull/674#issuecomment-713201392
This pull request **introduces 2 alerts** when merging 17bdb1612645a48dcca3bedf0ee70615135cae14 into 29dd73b5ce0253ccd0865aecb0986b4a42aec741 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-1faae7a365d553c6a867297d1f841b9232bb012a)
**new alerts:**
* 2 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] echobravopapa commented on a change in pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
echobravopapa commented on a change in pull request #674:
URL: https://github.com/apache/geode-native/pull/674#discussion_r505687688
##########
File path: clicache/acceptance-test/SNITests.cs
##########
@@ -127,5 +133,38 @@ public void ConnectionWithoutProxyFails()
Assert.Throws<NotConnectedException>(() => region.Put("1", "one"));
}
+
Review comment:
It would be great to have this test:
- bring up the system/cluster
- do a cache ops (put/get)
- drop the proxy
- attempt cache ops - wait for exception (EXPECTEXCEPTION in gtest or similar...)
- bring proxy back up
- then do verify cache ops (put/get)
----------------------------------------------------------------
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] lgtm-com[bot] commented on pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #674:
URL: https://github.com/apache/geode-native/pull/674#issuecomment-717397098
This pull request **introduces 8 alerts** when merging 63ddcc15eb353a0404ee7f93d3da89048bd5da0b into 52c8c162afadee9d3da0ea41defbea8474f85e3f - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-0c3128dda60a4020a1c064ab6e49a34b338aa237)
**new alerts:**
* 8 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] mmartell commented on a change in pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #674:
URL: https://github.com/apache/geode-native/pull/674#discussion_r508905378
##########
File path: clicache/acceptance-test/SNITests.cs
##########
@@ -127,5 +133,38 @@ public void ConnectionWithoutProxyFails()
Assert.Throws<NotConnectedException>(() => region.Put("1", "one"));
}
+
Review comment:
I've updated the test to use timeouts instead of catching exceptions. If you stop the proxy, the socket is closed by the proxy and the client throws a "Not Connected exception with message "No locators available", which can be caught. However, this requires restarting the proxy on the same port, which I'm not sure how to do from C#.
It seems just as good of a test to merely **pause** the proxy, which suspends the process and stops all network traffic. Then the proxy can merely be **unpaused**, which allows it to resume network communications on the same port. The only caveat to this simpler approach is that since the proxy doesn't actually close the socket, the client continues to retry the operation and never throws an exception. We use a wait with timeout to show the operation doesn't succeed while the proxy is paused, and another wait with timeout to show the operation succeeds as soon as the proxy is unpaused.
----------------------------------------------------------------
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] mmartell edited a comment on pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
mmartell edited a comment on pull request #674:
URL: https://github.com/apache/geode-native/pull/674#issuecomment-717981003
This last commit aligns the C++ and the C# SNI tests. Also, the DropProxy tests now mimic the behavior of the Java SNI tests by catching the NotConnectedException whenever the proxy is dropped. Also, much cleaner code now and addresses the unused variable gripes.
----------------------------------------------------------------
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] mmartell commented on a change in pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #674:
URL: https://github.com/apache/geode-native/pull/674#discussion_r512878424
##########
File path: clicache/acceptance-test/SNITests.cs
##########
@@ -127,5 +133,38 @@ public void ConnectionWithoutProxyFails()
Assert.Throws<NotConnectedException>(() => region.Put("1", "one"));
}
+
Review comment:
Figured out how to restart a stopped proxy on the original port. This allows us to catch the NotConnected exception just like in the Java client test.
----------------------------------------------------------------
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] moleske commented on a change in pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
moleske commented on a change in pull request #674:
URL: https://github.com/apache/geode-native/pull/674#discussion_r504859727
##########
File path: clicache/acceptance-test/SNITests.cs
##########
@@ -90,7 +91,18 @@ private int ParseProxyPort(string proxyString)
return Int32.Parse(portNumberString);
}
- [Fact]
+
+ //private Task PutAsync(IRegion<string, string> region)
Review comment:
Alas, poor commented code! I knew it, @mmartell : a line of async puts, of most excellent fancy: it hath borne me on its back a thousand times; and now, how abhorred in my imagination it is!
----------------------------------------------------------------
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] mmartell commented on pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
mmartell commented on pull request #674:
URL: https://github.com/apache/geode-native/pull/674#issuecomment-717981003
This last commit aligns the C++ and the C# SNI tests. Also, the DropProxy tests now mimic the behavior of the Java SNI tests by catching the NotConnectedException whenever the proxy is dropped. Also, much cleaner code now..
----------------------------------------------------------------
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] mmartell commented on a change in pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #674:
URL: https://github.com/apache/geode-native/pull/674#discussion_r505559467
##########
File path: clicache/acceptance-test/SNITests.cs
##########
@@ -90,7 +91,18 @@ private int ParseProxyPort(string proxyString)
return Int32.Parse(portNumberString);
}
- [Fact]
+
+ //private Task PutAsync(IRegion<string, string> region)
Review comment:
Lifted me for at least the remainder of this day you 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #674:
URL: https://github.com/apache/geode-native/pull/674#discussion_r510963460
##########
File path: clicache/acceptance-test/SNITests.cs
##########
@@ -127,5 +133,38 @@ public void ConnectionWithoutProxyFails()
Assert.Throws<NotConnectedException>(() => region.Put("1", "one"));
}
+
Review comment:
This is probably a correct test until such time as we resolve GEODE-8628. If it turns out NC should be honoring the read timeout value and throwing a timeout exception (what I would've expected), then when we fix it to do so properly, we can update this test to expect throw rather than just checking if the put is complete. @echobravopapa I'm in favor of resolving this bug and merging the new test, then writing one to actually shut down and restart the proxy and catch the proper exception etc. To my mind, these are different tests, both valid. Let us know what you think.
----------------------------------------------------------------
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] mmartell commented on a change in pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #674:
URL: https://github.com/apache/geode-native/pull/674#discussion_r508905378
##########
File path: clicache/acceptance-test/SNITests.cs
##########
@@ -127,5 +133,38 @@ public void ConnectionWithoutProxyFails()
Assert.Throws<NotConnectedException>(() => region.Put("1", "one"));
}
+
Review comment:
I've updated the test to use timeouts instead of catching exceptions. If you stop the proxy, the socket is closed by the proxy and the client throws a "Not Connected exception with message "No locators available", which can be caught. However, this requires restarting the proxy on the same port, which I'm not sure how to do from C#.
It seems just as good of a test to merely **pause** the proxy, which suspends the process and stops all network traffic. Then the proxy can merely be **unpaused**, which allows it to resume network communications on the same port. The only caveat to this simpler approach is that since the proxy doesn't actually close the socket, the client continues to retry the operation and never throws an exception.
----------------------------------------------------------------
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] mmartell merged pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
mmartell merged pull request #674:
URL: https://github.com/apache/geode-native/pull/674
----------------------------------------------------------------
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] lgtm-com[bot] commented on pull request #674: GEODE-8157: Add DropProxy acceptance test for .NET
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #674:
URL: https://github.com/apache/geode-native/pull/674#issuecomment-709371440
This pull request **introduces 2 alerts** when merging 1e127260f15fa8bc2d37642d7a1a0d6738f15edf into 7c30a8cdbf4de1d7ae34309f5094dea261a8ce1e - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-5e76a7b4d8c50503ff4f8e7f2e722888dd18a756)
**new alerts:**
* 2 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org