I realise it is unrelated to the PR, that's why I suggested the PR not remove the other code until it could be looked into, since it is related to the still-remaining stuff that did still _appear_ like it was used.
If the method is unused despite appearance then that is great, there is no issue and just a 30 second change improves things by removing the unpopulated field and making the method throw an appropriate exception, cutting off a rabbit hole and helping discouraging any future use. As is, your PR actually goes a good bit further and [re]moves some of the other bits that make the method appear used, so thats even better. I typically do consider potentially significantly broken message handling a blocker until its at least quickly looked at by someone with a clue to see the true impact, and I couldn't tell that, hence wanting someone who knows it to assess. I agree that its not a blocker now, though the minimal non-behavioural change does also seem like a no-brainer to include. On Wed, 15 Sept 2021 at 02:48, Clebert Suconic <clebert.suco...@gmail.com> wrote: > > that has nothing to do with the PR on large message, and it's totally > orthogonal to that change... > > As a matter of fact.. I don't even consider it a blocking... > > During the heavy refactoring on large messages supporting AMQP, I had > the getData() doing the right thing at some point, however as I kept > refacotring and improving it.. it ended up non used... and it became > dead. > > > The code is never used.. and it should not block a release... > > > Although I may send a PR today / tomorrow regardless.. per your > request.. but that's a totally optinal task Robbie > > On Tue, Sep 14, 2021 at 9:53 AM Robbie Gemmell <robbie.gemm...@gmail.com> > wrote: > > > > The dead code itself wasnt the issue raised, it's that the code > > created and operated on a buffer that remains even after removing the > > dead methods and is still being referenced elsewhere, though the field > > is never populated now so the using getData() method can only ever > > throw, and that method seems like it can be called. > > > > If you are saying that the getData() method can never actually be > > called despite appearance otherwise, then at minimum its impl should > > be changed to indicate that instead and stop referencing a field that > > is never populated. > > > > On Tue, 14 Sept 2021 at 14:00, Clebert Suconic > > <clebert.suco...@gmail.com> wrote: > > > > > > I wasn't sure what you were referring on that PR.... I thought you > > > still needed changes on the large message processing... > > > > > > > > > As for the dead code.. at some point I was reading the messages from > > > disk during reload of the journal. I then changed and I always have > > > the first packet on memory and I use that one to parse properties and > > > annotations. > > > > > > For converters... on AMQPLargeMessage.toCore() I will use the same > > > method used to deliver a large message (the Deliverer Context).. so > > > those dead code were forgotten there and not needed... there's nothing > > > to be done in that case. > > > > > > I thought I already mentioned that in the PR. > > > > > > On Tue, Sep 14, 2021 at 4:13 AM Robbie Gemmell <robbie.gemm...@gmail.com> > > > wrote: > > > > > > > > Revert what exactly? Reverting the PR this was raised on wouldn't > > > > actually change anything, since the apparent issue isnt to do with the > > > > changes made in the PR (other than it removed the dead code that looks > > > > like it shouldnt be dead, since the buffer it opened/closed is still > > > > referenced). Its some earlier change that must have made it the way it > > > > is. > > > > > > > > On Mon, 13 Sept 2021 at 22:43, Clebert Suconic > > > > <clebert.suco...@gmail.com> wrote: > > > > > > > > > > If the large message delivering in AMQP still an issue, I would rather > > > > > just revert the whole thing and keep the way it was before.. I thought > > > > > it was a simple change. > > > > > I'm addressing another issue I'm working on.. and I won't be able to > > > > > look into that > > > > > > > > > > > > > > > @Franz / @Robbie Gemmell ? > > > > > > > > > > > > > > > > > > > > On Mon, Sep 13, 2021 at 4:37 AM Robbie Gemmell > > > > > <robbie.gemm...@gmail.com> wrote: > > > > > > > > > > > > I this also still needs looked at: > > > > > > > > > > > > > I also think the issue noted in these comments should be > > > > > > > investigated > > > > > > > before another release occurs: > > > > > > > https://github.com/apache/activemq-artemis/pull/3711#issuecomment-913981275 > > > > > > > https://github.com/apache/activemq-artemis/pull/3711#issuecomment-914286613 > > > > > > > https://github.com/apache/activemq-artemis/pull/3711#discussion_r699397216 > > > > > > > > > > > > > On Fri, 10 Sept 2021 at 23:50, Clebert Suconic > > > > > > <clebert.suco...@gmail.com> wrote: > > > > > > > > > > > > > > I have merged the change on the mirror / paging fix.... > > > > > > > > > > > > > > > > > > > > > Before I release though I need to remove the CLI input I added.. > > > > > > > some > > > > > > > people complained about the retention input I asked during the > > > > > > > create.. (some users were saying their scripts were broken)... > > > > > > > although we have the --silent for such cases. > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Sep 9, 2021 at 10:51 PM Clebert Suconic > > > > > > > <clebert.suco...@gmail.com> wrote: > > > > > > > > > > > > > > > > I'm doing a retry on the same thread as the depage executor. > > > > > > > > > > > > > > > > > > > > > > > > I am finishing a test I'm writing and I will send the PR > > > > > > > > tomorrow (Friday) > > > > > > > > > > > > > > > > On Thu, Sep 9, 2021 at 12:46 PM Robbie Gemmell > > > > > > > > <robbie.gemm...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > I left some mostly trivial feedback on another PR merged > > > > > > > > > yesterday, > > > > > > > > > #3728, but there is one comment I think also needs looked at > > > > > > > > > first: > > > > > > > > > https://github.com/apache/activemq-artemis/pull/3728#discussion_r705417703 > > > > > > > > > > > > > > > > > > On Thu, 9 Sept 2021 at 15:26, Clebert Suconic > > > > > > > > > <clebert.suco...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > Let’s postpone until Monday while we investigate these. > > > > > > > > > > > > > > > > > > > > On Thu, Sep 9, 2021 at 9:01 AM Robbie Gemmell > > > > > > > > > > <robbie.gemm...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Franz believes he found the issue, side effects from an > > > > > > > > > > > earlier change > > > > > > > > > > > made months ago being hit now, exposed by use of the > > > > > > > > > > > affected method > > > > > > > > > > > in recent changes. > > > > > > > > > > > https://issues.apache.org/jira/browse/ARTEMIS-3465 > > > > > > > > > > > https://github.com/apache/activemq-artemis/pull/3731 > > > > > > > > > > > > > > > > > > > > > > I also think the issue noted in these comments should be > > > > > > > > > > > investigated > > > > > > > > > > > before another release occurs: > > > > > > > > > > > https://github.com/apache/activemq-artemis/pull/3711#issuecomment-913981275 > > > > > > > > > > > https://github.com/apache/activemq-artemis/pull/3711#issuecomment-914286613 > > > > > > > > > > > https://github.com/apache/activemq-artemis/pull/3711#discussion_r699397216 > > > > > > > > > > > > > > > > > > > > > > On Thu, 9 Sept 2021 at 12:57, Robbie Gemmell > > > > > > > > > > > <robbie.gemm...@gmail.com> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > Changes made on main recently (perhaps yesterday) look > > > > > > > > > > > > to have rather > > > > > > > > > > > > broken some things on expanded test runs (i.e not the > > > > > > > > > > > > push/PR subset), > > > > > > > > > > > > so that needs to be resolved first. > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 8 Sept 2021 at 22:48, Clebert Suconic > > > > > > > > > > > > <clebert.suco...@gmail.com> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Any problem if I did the release tomorrow? > > > > > > > > > > > > > > > > > > > > > > > > > > Anyone wants to include anything extra ? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Aug 20, 2021 at 3:10 PM Clebert Suconic < > > > > > > > > > > > clebert.suco...@gmail.com> > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > I would like to do a 2.19.0 release around Aug-30th. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Please help me out on merging stuff required before > > > > > > > > > > > > > > then. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if there's something (JIRA or PR) you would really > > > > > > > > > > > > > > like to include > > > > > > > > > > > > > > please mention it here.. (Last time someone > > > > > > > > > > > > > > mentioned me on the PR I > > > > > > > > > > > > > > missed the notification). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > > Clebert Suconic > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > Clebert Suconic > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > Clebert Suconic > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Clebert Suconic > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > Clebert Suconic > > > > > > > > > > > > > > > > > > > > -- > > > > > Clebert Suconic > > > > > > > > > > > > -- > > > Clebert Suconic > > > > -- > Clebert Suconic
