Re: Feedback request on minor JMX interface incompatibility for CASSANDRA-15937
Thanks for the feedback, given three +1s and no objections then I'll move on to get it merged. I'm satisfied with the code and David Capwell has completed his review. If another contributor with more experience with how JMX is used in the community could look (particularly somebody with operator experience) that would be greatly appreciated. On Thu, Aug 6, 2020 at 5:56 AM Benedict Elliott Smith wrote: > > +1 > > On 06/08/2020, 10:07, "Michael Semb Wever" wrote: > > > > I think the pragmatic thing to do is fix it now, and I'd strongly > > prefer to do that but wanted to check if there are any objections or > > things I hadn't considered? > > > +1 > > Thanks for giving this visibility and demonstrating we are serious about > the beta test cycle. > > - > To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org > For additional commands, e-mail: dev-h...@cassandra.apache.org > > > > > - > To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org > For additional commands, e-mail: dev-h...@cassandra.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org For additional commands, e-mail: dev-h...@cassandra.apache.org
Re: Feedback request on minor JMX interface incompatibility for CASSANDRA-15937
+1 On 06/08/2020, 10:07, "Michael Semb Wever" wrote: > I think the pragmatic thing to do is fix it now, and I'd strongly > prefer to do that but wanted to check if there are any objections or > things I hadn't considered? +1 Thanks for giving this visibility and demonstrating we are serious about the beta test cycle. - To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org For additional commands, e-mail: dev-h...@cassandra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org For additional commands, e-mail: dev-h...@cassandra.apache.org
Re: Feedback request on minor JMX interface incompatibility for CASSANDRA-15937
> I think the pragmatic thing to do is fix it now, and I'd strongly > prefer to do that but wanted to check if there are any objections or > things I hadn't considered? +1 Thanks for giving this visibility and demonstrating we are serious about the beta test cycle. - To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org For additional commands, e-mail: dev-h...@cassandra.apache.org
Re: Feedback request on minor JMX interface incompatibility for CASSANDRA-15937
> > I think the pragmatic thing to do is fix it now, and I'd strongly > prefer to do that but wanted to check if there are any objections or > things I hadn't considered? +1 from me, should fix. On Wed, Jul 29, 2020 at 11:59 AM Jon Meredith wrote: > Following up on Mick's email about interface incompatible changes, > CASSANDRA-15937 is ready for review. In my opinion, this just fixes > some bugs in CASSANDRA-7544 patch that slipped through the original > review, but before we review & merge fixes I wanted to ask if anybody > had any objections to merging it at this stage. > > The ticket has more details, but to recap when InetAddressAndPort was > introduced in CASSANDRA-7544 so that nodes can share the same IP > address with different storage ports, new versions of status Mbeans > were added that included the port so as not to break anything that > didn't expect a port number. Unfortunately the change introduced > inconsistencies in how the host address is formatted between the old > and new versions. It also updated some per-connection metrics which > are also affected. > > Before CASSANDRA-7544, describing host addresses either used > InetAddress.toString which formats as "/" or > InetAddress.getHostAddress which just provides a numeric address. > InetAddressAndPort was introduced with only a toString() method that > just provided the numeric address and port number, but did not provide > an equivalent getHostAddress/getHostAddressAndPort methods. The > updated versions of all the JMX endpoints all called toString and > dropped the resolved names. > > CASSANDRA-15937 adds an implementation of getHostAddressAndPort and > reverts the getHostAddress to toString conversions, giving consistent > formatting between the two versions, but with the port number. This > has the following impact (with a couple of other minor bugs fixed). > > 1) AllEndpointStats, SimpleStates, Connection message tracking, > TimeoutsPerHost now include the [host]/ip:port in the WithPort version > 2) Correct LoadMap to omit port (fixes backward compatibility) > LoadMapWithPort to include port (which was missing) > 3) Ownership - update WithPort to include the [host]/ip:port version > 4) Scores - update WithPort to [host]/ip:port version > > It's a shame we didn't discover/resolve it before we cut beta1 as it > does change things by introducing "[host]/" in some places, however > this is only in 4.0 related fields that were just introduced. > > The alternatives to merging I can see are > > 1) Do nothing for the 4.0 release and have the new WithPort versions > broken until we fix in a new major. > 2) Introduce a third-version of the naming with things fixed > (WithPort_2 or something awful) > 3) Reduce scope - don't 'fix' all the bare host addresses, just fix > LoadMap as it is the only one that is actually *wrong*, rather than > different. Perhaps the resolved name before the slash is annoying to > people. > > I think the pragmatic thing to do is fix it now, and I'd strongly > prefer to do that but wanted to check if there are any objections or > things I hadn't considered? > > Cheers, Jon. > > - > To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org > For additional commands, e-mail: dev-h...@cassandra.apache.org > >
Feedback request on minor JMX interface incompatibility for CASSANDRA-15937
Following up on Mick's email about interface incompatible changes, CASSANDRA-15937 is ready for review. In my opinion, this just fixes some bugs in CASSANDRA-7544 patch that slipped through the original review, but before we review & merge fixes I wanted to ask if anybody had any objections to merging it at this stage. The ticket has more details, but to recap when InetAddressAndPort was introduced in CASSANDRA-7544 so that nodes can share the same IP address with different storage ports, new versions of status Mbeans were added that included the port so as not to break anything that didn't expect a port number. Unfortunately the change introduced inconsistencies in how the host address is formatted between the old and new versions. It also updated some per-connection metrics which are also affected. Before CASSANDRA-7544, describing host addresses either used InetAddress.toString which formats as "/" or InetAddress.getHostAddress which just provides a numeric address. InetAddressAndPort was introduced with only a toString() method that just provided the numeric address and port number, but did not provide an equivalent getHostAddress/getHostAddressAndPort methods. The updated versions of all the JMX endpoints all called toString and dropped the resolved names. CASSANDRA-15937 adds an implementation of getHostAddressAndPort and reverts the getHostAddress to toString conversions, giving consistent formatting between the two versions, but with the port number. This has the following impact (with a couple of other minor bugs fixed). 1) AllEndpointStats, SimpleStates, Connection message tracking, TimeoutsPerHost now include the [host]/ip:port in the WithPort version 2) Correct LoadMap to omit port (fixes backward compatibility) LoadMapWithPort to include port (which was missing) 3) Ownership - update WithPort to include the [host]/ip:port version 4) Scores - update WithPort to [host]/ip:port version It's a shame we didn't discover/resolve it before we cut beta1 as it does change things by introducing "[host]/" in some places, however this is only in 4.0 related fields that were just introduced. The alternatives to merging I can see are 1) Do nothing for the 4.0 release and have the new WithPort versions broken until we fix in a new major. 2) Introduce a third-version of the naming with things fixed (WithPort_2 or something awful) 3) Reduce scope - don't 'fix' all the bare host addresses, just fix LoadMap as it is the only one that is actually *wrong*, rather than different. Perhaps the resolved name before the slash is annoying to people. I think the pragmatic thing to do is fix it now, and I'd strongly prefer to do that but wanted to check if there are any objections or things I hadn't considered? Cheers, Jon. - To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org For additional commands, e-mail: dev-h...@cassandra.apache.org
