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

Reply via email to