You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/11/16 16:03:30 UTC
[GitHub] [qpid-dispatch] ganeshmurthy opened a new pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
ganeshmurthy opened a new pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441
…rs are 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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441#discussion_r752641101
##########
File path: tests/system_tests_distribution.py
##########
@@ -3826,6 +3823,7 @@ def __init__(self,
self.error = None
self.messages_per_sender = 100
Review comment:
I am curious to find out why this test is failing. I am sure the failure has nothing to do with the number of messages sent. I want to push this logging change to main and analyze the debug output when the test fails.
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441#discussion_r753511228
##########
File path: tests/system_tests_distribution.py
##########
@@ -3972,19 +3958,25 @@ def on_link_opening(self, event):
if self.n_waypoint_receivers < 2 :
self.waypoints[self.n_waypoint_receivers]['receiver'] = event.receiver
self.n_waypoint_receivers += 1
+ # Create the senders after the waypoint receiver links have been opened.
+ if self.n_waypoint_receivers == 2:
+ for i in range(len(self.sender_connections)):
+ cnx = self.sender_connections[i]
+ sender = self.senders[i]
+ sender['sender'] = event.container.create_sender(cnx,
+ self.destination,
+ name=link_name())
+ sender['to_send'] = self.messages_per_sender
+ sender['n_sent'] = 0
def on_sendable(self, event):
- self.debug_print("on_sendable ------------------------------")
for index in range(len(self.senders)) :
sender = self.senders[index]
if event.sender == sender['sender'] :
- self.debug_print(" client sender %d" % index)
if sender['n_sent'] < sender['to_send'] :
self.debug_print(" sending %d" % sender['to_send'])
self.send_from_client(sender['sender'], sender['to_send'], index)
sender['n_sent'] = sender['to_send'] # n_sent = n_to_send
Review comment:
If send_from_client() exited because of lack of credit, the debug_print we added will print the n_sent. This additional logging will tell us if the client could not send the desired number of messages due to the lack of credit
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit merged pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441#discussion_r752662207
##########
File path: tests/system_tests_distribution.py
##########
@@ -3910,11 +3908,9 @@ def send_from_client(self, sender, n_messages, sender_index):
n_sent += 1
self.n_sent += 1
self.n_transitions += 1
- self.debug_print("send_from_client -- sender: %d n_sent: %d" % (sender_index, n_sent))
+ self.debug_print("send_from_client -- sender: %d n_sent: %d" % (sender_index, n_sent))
Review comment:
Actually, no. The loop in send_from_waypoint prints the message_content_number, so I want to keep the print inside the loop
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441#issuecomment-970420168
This PR enables debug logging so we can figure out what is going on in the Travis environment when this test fails.
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441#issuecomment-973675453
# [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1441](https://codecov.io/gh/apache/qpid-dispatch/pull/1441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60f2763) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/210571f9c354961fdc636dedc9e440b877047afa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (210571f) will **decrease** coverage by `0.02%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/qpid-dispatch/pull/1441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## main #1441 +/- ##
==========================================
- Coverage 84.71% 84.69% -0.03%
==========================================
Files 116 116
Lines 28619 28619
==========================================
- Hits 24244 24238 -6
- Misses 4375 4381 +6
```
| [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `89.05% <0.00%> (-1.06%)` | :arrow_down: |
| [src/router\_core/router\_core.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3JvdXRlcl9jb3JlLmM=) | `86.57% <0.00%> (-0.48%)` | :arrow_down: |
| [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `93.25% <0.00%> (-0.20%)` | :arrow_down: |
| [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21lc3NhZ2UuYw==) | `87.42% <0.00%> (-0.07%)` | :arrow_down: |
| [src/router\_core/forwarder.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2ZvcndhcmRlci5j) | `93.46% <0.00%> (+0.39%)` | :arrow_up: |
| [src/hash.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2hhc2guYw==) | `80.00% <0.00%> (+0.46%)` | :arrow_up: |
| [src/router\_core/modules/edge\_router/edge\_mgmt.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvZWRnZV9tZ210LmM=) | `85.14% <0.00%> (+0.99%)` | :arrow_up: |
| [...router\_core/modules/edge\_router/link\_route\_proxy.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvbGlua19yb3V0ZV9wcm94eS5j) | `82.84% <0.00%> (+4.14%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [210571f...60f2763](https://codecov.io/gh/apache/qpid-dispatch/pull/1441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1441: DISPATCH-2258: Cleaned up log messages, created senders after receive…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1441:
URL: https://github.com/apache/qpid-dispatch/pull/1441#discussion_r751355815
##########
File path: tests/system_tests_distribution.py
##########
@@ -3884,7 +3882,7 @@ def __init__(self,
n_links_per_message = 4
self.n_expected_transitions = len(self.senders) * self.messages_per_sender * n_links_per_message
- self.debug = False
+ self.debug = debug
self.test_name = test_name
Review comment:
Since github won't let me comment on lines that haven't been changed: this is regards to the debug_print() function: add a flush=True to the print statement: print(message, flush=True). unittest has a tendency to not flush stdout if a failure is hit.
##########
File path: tests/system_tests_distribution.py
##########
@@ -3826,6 +3823,7 @@ def __init__(self,
self.error = None
self.messages_per_sender = 100
Review comment:
Non-patch observation: is 100 messages *really* necessary? CI systems tend to be painfully slow. Can the same degree of confidence that this works be achieved by say 20 messages?
##########
File path: tests/system_tests_distribution.py
##########
@@ -3910,11 +3908,9 @@ def send_from_client(self, sender, n_messages, sender_index):
n_sent += 1
self.n_sent += 1
self.n_transitions += 1
- self.debug_print("send_from_client -- sender: %d n_sent: %d" % (sender_index, n_sent))
+ self.debug_print("send_from_client -- sender: %d n_sent: %d" % (sender_index, n_sent))
Review comment:
Did you also want to do the same thing (move the debug print out of the loop) for the send_from_waypoint() method below? Just asking...
##########
File path: tests/system_tests_distribution.py
##########
@@ -3972,19 +3958,25 @@ def on_link_opening(self, event):
if self.n_waypoint_receivers < 2 :
self.waypoints[self.n_waypoint_receivers]['receiver'] = event.receiver
self.n_waypoint_receivers += 1
+ # Create the senders after the waypoint receiver links have been opened.
+ if self.n_waypoint_receivers == 2:
+ for i in range(len(self.sender_connections)):
+ cnx = self.sender_connections[i]
+ sender = self.senders[i]
+ sender['sender'] = event.container.create_sender(cnx,
+ self.destination,
+ name=link_name())
+ sender['to_send'] = self.messages_per_sender
+ sender['n_sent'] = 0
def on_sendable(self, event):
- self.debug_print("on_sendable ------------------------------")
for index in range(len(self.senders)) :
sender = self.senders[index]
if event.sender == sender['sender'] :
- self.debug_print(" client sender %d" % index)
if sender['n_sent'] < sender['to_send'] :
self.debug_print(" sending %d" % sender['to_send'])
self.send_from_client(sender['sender'], sender['to_send'], index)
sender['n_sent'] = sender['to_send'] # n_sent = n_to_send
Review comment:
wait a sec... can't send_from_client exit with < to_send if credit runs out? This seems wrong. Maybe send_from_client should be incrementing sender['n_sent'] rather than having this line assume sender['to_send'] was actually sent.
And don't get me started on why the heck this test is using maps (sender['n_sent']) rather than a class (sender.n_sent) all over this test. It's like this test was purposely written to be easy to break and maximize reader confusion.
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org