Opened 7 years ago

Closed 6 years ago

#4262 defect closed fixed (fixed)

Multicast documentation is misleading

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: normal Milestone:
Component: core Keywords: documentation, easy
Cc: jesstess Branch: branches/multicast-doc-4262
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst, frap

Description

  1. The client example in the multicast docs doesn't really explain that it will not be able to receive multicast messages, only send them. The second example should therefore probably also have a joinGroup() in the example.
  1. The server example shows replying via UDP, rather than multicast. There should be some explanation about how it's possible to do both.
  1. The comment in the server example about doing .join() may be wrong, it should probably also be a joinGroup().

Attachments (2)

patch-multicastdoc-4262.patch (8.2 KB) - added by Andrés Gasson 6 years ago.
patch-mcastdoc-4262-2.patch (1.7 KB) - added by Andrés Gasson 6 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by Glyph

Owner: changed from Glyph to Itamar Turner-Trauring

Unfortunately I don't really know anything about multicast; you'll have to find someone who does to fix the docs.

comment:2 Changed 6 years ago by Andrés Gasson

Owner: changed from Itamar Turner-Trauring to Andrés Gasson
Status: newassigned

Changed 6 years ago by Andrés Gasson

comment:3 Changed 6 years ago by Andrés Gasson

Keywords: review added

1: Updated - second example was using UDP unicast for reply so have changed to multicast

  1. Not sure what you mean here via Ethernet (L2) multicast? Multicast for IP needs a transport and that is UDP?
  2. have updated as well as changed to xmtml documentation to better reflect how multicast works

I aslo noticed there are no tests for multicast transport and its special methods like JoinGroup, setTTL - should I look at trying to do that? Would I have to mock these?

comment:4 Changed 6 years ago by Thijs Triemstra

Owner: Andrés Gasson deleted
Status: assignednew

Thanks, putting it up for review (by assigning it to nobody, see ReviewProcess#Authors:Howtogetyourchangereviewed). Please ask on the #twisted irc channel about the missing tests and/or open a new ticket for that.

comment:5 Changed 6 years ago by Itamar Turner-Trauring

There are actually tests for multicast in twisted.test.test_udp.

comment:6 in reply to:  5 Changed 6 years ago by Andrés Gasson

Author: frap

Replying to itamar:

There are actually tests for multicast in twisted.test.test_udp.

yeah my bad didnt realise test for core functionality were above core directory. Hopefully rest of changes are suitable

comment:7 Changed 6 years ago by itamarst

Author: frapitamarst, frap
Branch: branches/multicast-doc-4262

(In [33099]) Branching to 'multicast-doc-4262'

comment:8 Changed 6 years ago by Itamar Turner-Trauring

I've simplified and edited the patch enough that I probably shouldn't be reviewer, so leaving with review keyword.

Changed 6 years ago by Andrés Gasson

Attachment: patch-mcastdoc-4262-2.patch added

comment:9 Changed 6 years ago by Andrés Gasson

Owner: set to Itamar Turner-Trauring

Changes look good, I have just edited the comments of the code samples to make them clearer as the client wasn't unicasting back. Also changed the clients datagramReceived method so it prints the address so people runnig the code can see they receive their own ping.

If you are happy with the changes then please merge.

comment:10 Changed 6 years ago by Itamar Turner-Trauring

Owner: Itamar Turner-Trauring deleted

Patch applied. I'll see if I can get anyone else to glance at this.

comment:11 Changed 6 years ago by jesstess

Owner: set to jesstess

comment:12 Changed 6 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: changed from jesstess to Itamar Turner-Trauring

This looks great! Thanks for working on this frap and itamar. A few tiny things:

  • "This in contrast with normal" ==> "This is in contrast with normal"
  • "(the IPv4 range 224.0.0.0 to 239.255.255.255)" ==> "(in the IPv4 range 224.0.0.0 to 239.255.255.255)", or "(the IPv4 range is 224.0.0.0 to 239.255.255.255)" ?

Other than that, I think this is ready to merge.

comment:13 Changed 6 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [33121]) Merge multicast-doc-4262.

Author: frap, itamar Review: jesstess, itamar Fixes: #4262

Improve the multicast documentation, with better explanations and sample code.

Note: See TracTickets for help on using tickets.