You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@dubbo.apache.org by Imteyaz Khan <kh...@gmail.com> on 2018/12/20 12:05:29 UTC
AccessLogFilter simple date format instance creation change PR
Hi All,
I have raised a PR <https://github.com/apache/incubator-dubbo/pull/3027>for
issue 3026 <https://github.com/apache/incubator-dubbo/issues/3026>. In this
PR , I am reducing the number of SimpleDateFormat object creation from
each log message to per thread wise and reusing it. Please review it.
https://github.com/apache/incubator-dubbo/pull/3027
Re: AccessLogFilter simple date format instance creation change PR
Posted by Imteyaz Khan <kh...@gmail.com>.
Thanks. Sure looking into it.
On Mon, Jan 21, 2019 at 8:08 AM Ian Luo <ia...@gmail.com> wrote:
> Imteyaz,
>
> I am on your pull request. I drop one comment, would you mind to take a
> look?
>
> Thanks,
> -Ian.
>
>
> On Sat, Jan 19, 2019 at 3:58 AM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
> > Hi All,
> > PR which I raised mentioned in email thread, due to recent changes it
> > encounters some conflicts and after resolving and taking latest pull it
> was
> > showing 500+ file chages (It might I have not done it right way :). To
> > avoide confusion I have closed existing one and created new on. This new
> > *PR
> > <https://github.com/apache/incubator-dubbo/pull/3274>*has all the
> changes
> > till date which I have incorporated as review feedback.
> >
> > In my old PR <https://github.com/apache/incubator-dubbo/pull/3090>there
> > was
> > request changes asked by @zonghaishang <https://github.com/zonghaishang>
> > (Not
> > thread safe implementation), which I beleive I have addressed as there
> was
> > also about making thread safe and reducing the creation of
> > SimpleDateFormat.
> >
> > Old PR link : https://github.com/apache/incubator-dubbo/pull/3090
> > New PR link :https://github.com/apache/incubator-dubbo/pull/3274
> >
> > Once again sorry for the inconvenience, and would request review my PR.
> >
> > Regards
> > Imteyaz
> >
> > On Sat, Dec 29, 2018 at 11:58 AM yuhang xiu <ca...@gmail.com> wrote:
> >
> > > Hi, @beiwei @khan
> > >
> > > There have been a lot of things in recent work.
> > > After the end of New Year's Day, I believe that there will be more time
> > to
> > > work in the community, I am very sorry.
> > >
> > > I will dispose of this PR next week. @khan
> > > :)
> > >
> > > Ian Luo <ia...@gmail.com> 于2018年12月29日周六 上午11:14写道：
> > >
> > > > I took a glance at the latest change. It looks good to me since we
> > avoid
> > > of
> > > > using thread-local and new instance for every incoming request,
> instead
> > > one
> > > > singleton date formatter runs in one single thread, which I totally
> > agree
> > > > with.
> > > >
> > > > Let's wait for Yuhang's review comments :)
> > > >
> > > > -Ian.
> > > >
> > > >
> > > > On Sat, Dec 29, 2018 at 12:11 AM Imteyaz Khan <
> khan.imteyaz@gmail.com>
> > > > wrote:
> > > >
> > > > > Due to my own confusion and bad I found I found my PR is containing
> > > more
> > > > > commits and more file modification other than mine. So avoid the
> > > > confusion
> > > > > I created new PR and close the old one. This PR containing the all
> > the
> > > > > review comments from old PR. Please review mine this PR
> > > > > <https://github.com/apache/incubator-dubbo/pull/3090>
> > > > >
> > > > >
> > > > > https://github.com/apache/incubator-dubbo/pull/3090
> > > > >
> > > > > Sorry for the inconvenience caused.
> > > > >
> > > > >
> > > > > On Thu, Dec 27, 2018 at 3:24 PM yuhang xiu <ca...@gmail.com>
> > wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > I simply checked your pr and I agree with you. I will review it.
> > > > > > Nice work!
> > > > > >
> > > > > > Imteyaz Khan <kh...@gmail.com> 于2018年12月27日周四 下午5:35写道：
> > > > > >
> > > > > > > Hi All,
> > > > > > > I have raised a separate PR for the same issue 3026.
> > > > > > >
> > > > > > > https://github.com/apache/incubator-dubbo/pull/3080
> > > > > > >
> > > > > > > As my old PR was containing may commits of others and reviewer
> > > would
> > > > > like
> > > > > > > me to clear those commits and I tried hard to achieve the same
> > but
> > > > > failed
> > > > > > > so landed up creating new PR
> > > > > > > <https://github.com/apache/incubator-dubbo/pull/3080>.
> > > > > > >
> > > > > > > Please note :: older PR
> > > > > > > <https://github.com/apache/incubator-dubbo/pull/3027> 3027 has
> > > been
> > > > > > closed
> > > > > > > but all the comments are applicable here.
> > > > > > >
> > > > > > > On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <
> > > khan.imteyaz@gmail.com
> > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi All,
> > > > > > > > I have raised a PR <
> > > > > > > https://github.com/apache/incubator-dubbo/pull/3027>for
> > > > > > > > issue 3026 <
> > > https://github.com/apache/incubator-dubbo/issues/3026
> > > > >.
> > > > > In
> > > > > > > > this PR , I am reducing the number of SimpleDateFormat
> object
> > > > > creation
> > > > > > > > from each log message to per thread wise and reusing it.
> Please
> > > > > review
> > > > > > > it.
> > > > > > > >
> > > > > > > > https://github.com/apache/incubator-dubbo/pull/3027
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: AccessLogFilter simple date format instance creation change PR
Posted by Ian Luo <ia...@gmail.com>.
Imteyaz,
I am on your pull request. I drop one comment, would you mind to take a
look?
Thanks,
-Ian.
On Sat, Jan 19, 2019 at 3:58 AM Imteyaz Khan <kh...@gmail.com> wrote:
> Hi All,
> PR which I raised mentioned in email thread, due to recent changes it
> encounters some conflicts and after resolving and taking latest pull it was
> showing 500+ file chages (It might I have not done it right way :). To
> avoide confusion I have closed existing one and created new on. This new
> *PR
> <https://github.com/apache/incubator-dubbo/pull/3274>*has all the changes
> till date which I have incorporated as review feedback.
>
> In my old PR <https://github.com/apache/incubator-dubbo/pull/3090>there
> was
> request changes asked by @zonghaishang <https://github.com/zonghaishang>
> (Not
> thread safe implementation), which I beleive I have addressed as there was
> also about making thread safe and reducing the creation of
> SimpleDateFormat.
>
> Old PR link : https://github.com/apache/incubator-dubbo/pull/3090
> New PR link :https://github.com/apache/incubator-dubbo/pull/3274
>
> Once again sorry for the inconvenience, and would request review my PR.
>
> Regards
> Imteyaz
>
> On Sat, Dec 29, 2018 at 11:58 AM yuhang xiu <ca...@gmail.com> wrote:
>
> > Hi, @beiwei @khan
> >
> > There have been a lot of things in recent work.
> > After the end of New Year's Day, I believe that there will be more time
> to
> > work in the community, I am very sorry.
> >
> > I will dispose of this PR next week. @khan
> > :)
> >
> > Ian Luo <ia...@gmail.com> 于2018年12月29日周六 上午11:14写道：
> >
> > > I took a glance at the latest change. It looks good to me since we
> avoid
> > of
> > > using thread-local and new instance for every incoming request, instead
> > one
> > > singleton date formatter runs in one single thread, which I totally
> agree
> > > with.
> > >
> > > Let's wait for Yuhang's review comments :)
> > >
> > > -Ian.
> > >
> > >
> > > On Sat, Dec 29, 2018 at 12:11 AM Imteyaz Khan <kh...@gmail.com>
> > > wrote:
> > >
> > > > Due to my own confusion and bad I found I found my PR is containing
> > more
> > > > commits and more file modification other than mine. So avoid the
> > > confusion
> > > > I created new PR and close the old one. This PR containing the all
> the
> > > > review comments from old PR. Please review mine this PR
> > > > <https://github.com/apache/incubator-dubbo/pull/3090>
> > > >
> > > >
> > > > https://github.com/apache/incubator-dubbo/pull/3090
> > > >
> > > > Sorry for the inconvenience caused.
> > > >
> > > >
> > > > On Thu, Dec 27, 2018 at 3:24 PM yuhang xiu <ca...@gmail.com>
> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > I simply checked your pr and I agree with you. I will review it.
> > > > > Nice work!
> > > > >
> > > > > Imteyaz Khan <kh...@gmail.com> 于2018年12月27日周四 下午5:35写道：
> > > > >
> > > > > > Hi All,
> > > > > > I have raised a separate PR for the same issue 3026.
> > > > > >
> > > > > > https://github.com/apache/incubator-dubbo/pull/3080
> > > > > >
> > > > > > As my old PR was containing may commits of others and reviewer
> > would
> > > > like
> > > > > > me to clear those commits and I tried hard to achieve the same
> but
> > > > failed
> > > > > > so landed up creating new PR
> > > > > > <https://github.com/apache/incubator-dubbo/pull/3080>.
> > > > > >
> > > > > > Please note :: older PR
> > > > > > <https://github.com/apache/incubator-dubbo/pull/3027> 3027 has
> > been
> > > > > closed
> > > > > > but all the comments are applicable here.
> > > > > >
> > > > > > On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <
> > khan.imteyaz@gmail.com
> > > >
> > > > > > wrote:
> > > > > >
> > > > > > > Hi All,
> > > > > > > I have raised a PR <
> > > > > > https://github.com/apache/incubator-dubbo/pull/3027>for
> > > > > > > issue 3026 <
> > https://github.com/apache/incubator-dubbo/issues/3026
> > > >.
> > > > In
> > > > > > > this PR , I am reducing the number of SimpleDateFormat object
> > > > creation
> > > > > > > from each log message to per thread wise and reusing it. Please
> > > > review
> > > > > > it.
> > > > > > >
> > > > > > > https://github.com/apache/incubator-dubbo/pull/3027
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: AccessLogFilter simple date format instance creation change PR
Posted by Imteyaz Khan <kh...@gmail.com>.
Hi All,
PR which I raised mentioned in email thread, due to recent changes it
encounters some conflicts and after resolving and taking latest pull it was
showing 500+ file chages (It might I have not done it right way :). To
avoide confusion I have closed existing one and created new on. This new *PR
<https://github.com/apache/incubator-dubbo/pull/3274>*has all the changes
till date which I have incorporated as review feedback.
In my old PR <https://github.com/apache/incubator-dubbo/pull/3090>there was
request changes asked by @zonghaishang <https://github.com/zonghaishang> (Not
thread safe implementation), which I beleive I have addressed as there was
also about making thread safe and reducing the creation of SimpleDateFormat.
Old PR link : https://github.com/apache/incubator-dubbo/pull/3090
New PR link :https://github.com/apache/incubator-dubbo/pull/3274
Once again sorry for the inconvenience, and would request review my PR.
Regards
Imteyaz
On Sat, Dec 29, 2018 at 11:58 AM yuhang xiu <ca...@gmail.com> wrote:
> Hi, @beiwei @khan
>
> There have been a lot of things in recent work.
> After the end of New Year's Day, I believe that there will be more time to
> work in the community, I am very sorry.
>
> I will dispose of this PR next week. @khan
> :)
>
> Ian Luo <ia...@gmail.com> 于2018年12月29日周六 上午11:14写道：
>
> > I took a glance at the latest change. It looks good to me since we avoid
> of
> > using thread-local and new instance for every incoming request, instead
> one
> > singleton date formatter runs in one single thread, which I totally agree
> > with.
> >
> > Let's wait for Yuhang's review comments :)
> >
> > -Ian.
> >
> >
> > On Sat, Dec 29, 2018 at 12:11 AM Imteyaz Khan <kh...@gmail.com>
> > wrote:
> >
> > > Due to my own confusion and bad I found I found my PR is containing
> more
> > > commits and more file modification other than mine. So avoid the
> > confusion
> > > I created new PR and close the old one. This PR containing the all the
> > > review comments from old PR. Please review mine this PR
> > > <https://github.com/apache/incubator-dubbo/pull/3090>
> > >
> > >
> > > https://github.com/apache/incubator-dubbo/pull/3090
> > >
> > > Sorry for the inconvenience caused.
> > >
> > >
> > > On Thu, Dec 27, 2018 at 3:24 PM yuhang xiu <ca...@gmail.com> wrote:
> > >
> > > > Hi,
> > > >
> > > > I simply checked your pr and I agree with you. I will review it.
> > > > Nice work!
> > > >
> > > > Imteyaz Khan <kh...@gmail.com> 于2018年12月27日周四 下午5:35写道：
> > > >
> > > > > Hi All,
> > > > > I have raised a separate PR for the same issue 3026.
> > > > >
> > > > > https://github.com/apache/incubator-dubbo/pull/3080
> > > > >
> > > > > As my old PR was containing may commits of others and reviewer
> would
> > > like
> > > > > me to clear those commits and I tried hard to achieve the same but
> > > failed
> > > > > so landed up creating new PR
> > > > > <https://github.com/apache/incubator-dubbo/pull/3080>.
> > > > >
> > > > > Please note :: older PR
> > > > > <https://github.com/apache/incubator-dubbo/pull/3027> 3027 has
> been
> > > > closed
> > > > > but all the comments are applicable here.
> > > > >
> > > > > On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <
> khan.imteyaz@gmail.com
> > >
> > > > > wrote:
> > > > >
> > > > > > Hi All,
> > > > > > I have raised a PR <
> > > > > https://github.com/apache/incubator-dubbo/pull/3027>for
> > > > > > issue 3026 <
> https://github.com/apache/incubator-dubbo/issues/3026
> > >.
> > > In
> > > > > > this PR , I am reducing the number of SimpleDateFormat object
> > > creation
> > > > > > from each log message to per thread wise and reusing it. Please
> > > review
> > > > > it.
> > > > > >
> > > > > > https://github.com/apache/incubator-dubbo/pull/3027
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: AccessLogFilter simple date format instance creation change PR
Posted by yuhang xiu <ca...@gmail.com>.
Hi, @beiwei @khan
There have been a lot of things in recent work.
After the end of New Year's Day, I believe that there will be more time to
work in the community, I am very sorry.
I will dispose of this PR next week. @khan
:)
Ian Luo <ia...@gmail.com> 于2018年12月29日周六 上午11:14写道：
> I took a glance at the latest change. It looks good to me since we avoid of
> using thread-local and new instance for every incoming request, instead one
> singleton date formatter runs in one single thread, which I totally agree
> with.
>
> Let's wait for Yuhang's review comments :)
>
> -Ian.
>
>
> On Sat, Dec 29, 2018 at 12:11 AM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
> > Due to my own confusion and bad I found I found my PR is containing more
> > commits and more file modification other than mine. So avoid the
> confusion
> > I created new PR and close the old one. This PR containing the all the
> > review comments from old PR. Please review mine this PR
> > <https://github.com/apache/incubator-dubbo/pull/3090>
> >
> >
> > https://github.com/apache/incubator-dubbo/pull/3090
> >
> > Sorry for the inconvenience caused.
> >
> >
> > On Thu, Dec 27, 2018 at 3:24 PM yuhang xiu <ca...@gmail.com> wrote:
> >
> > > Hi,
> > >
> > > I simply checked your pr and I agree with you. I will review it.
> > > Nice work!
> > >
> > > Imteyaz Khan <kh...@gmail.com> 于2018年12月27日周四 下午5:35写道：
> > >
> > > > Hi All,
> > > > I have raised a separate PR for the same issue 3026.
> > > >
> > > > https://github.com/apache/incubator-dubbo/pull/3080
> > > >
> > > > As my old PR was containing may commits of others and reviewer would
> > like
> > > > me to clear those commits and I tried hard to achieve the same but
> > failed
> > > > so landed up creating new PR
> > > > <https://github.com/apache/incubator-dubbo/pull/3080>.
> > > >
> > > > Please note :: older PR
> > > > <https://github.com/apache/incubator-dubbo/pull/3027> 3027 has been
> > > closed
> > > > but all the comments are applicable here.
> > > >
> > > > On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <khan.imteyaz@gmail.com
> >
> > > > wrote:
> > > >
> > > > > Hi All,
> > > > > I have raised a PR <
> > > > https://github.com/apache/incubator-dubbo/pull/3027>for
> > > > > issue 3026 <https://github.com/apache/incubator-dubbo/issues/3026
> >.
> > In
> > > > > this PR , I am reducing the number of SimpleDateFormat object
> > creation
> > > > > from each log message to per thread wise and reusing it. Please
> > review
> > > > it.
> > > > >
> > > > > https://github.com/apache/incubator-dubbo/pull/3027
> > > > >
> > > >
> > >
> >
>
Re: AccessLogFilter simple date format instance creation change PR
Posted by Ian Luo <ia...@gmail.com>.
I took a glance at the latest change. It looks good to me since we avoid of
using thread-local and new instance for every incoming request, instead one
singleton date formatter runs in one single thread, which I totally agree
with.
Let's wait for Yuhang's review comments :)
-Ian.
On Sat, Dec 29, 2018 at 12:11 AM Imteyaz Khan <kh...@gmail.com>
wrote:
> Due to my own confusion and bad I found I found my PR is containing more
> commits and more file modification other than mine. So avoid the confusion
> I created new PR and close the old one. This PR containing the all the
> review comments from old PR. Please review mine this PR
> <https://github.com/apache/incubator-dubbo/pull/3090>
>
>
> https://github.com/apache/incubator-dubbo/pull/3090
>
> Sorry for the inconvenience caused.
>
>
> On Thu, Dec 27, 2018 at 3:24 PM yuhang xiu <ca...@gmail.com> wrote:
>
> > Hi,
> >
> > I simply checked your pr and I agree with you. I will review it.
> > Nice work!
> >
> > Imteyaz Khan <kh...@gmail.com> 于2018年12月27日周四 下午5:35写道：
> >
> > > Hi All,
> > > I have raised a separate PR for the same issue 3026.
> > >
> > > https://github.com/apache/incubator-dubbo/pull/3080
> > >
> > > As my old PR was containing may commits of others and reviewer would
> like
> > > me to clear those commits and I tried hard to achieve the same but
> failed
> > > so landed up creating new PR
> > > <https://github.com/apache/incubator-dubbo/pull/3080>.
> > >
> > > Please note :: older PR
> > > <https://github.com/apache/incubator-dubbo/pull/3027> 3027 has been
> > closed
> > > but all the comments are applicable here.
> > >
> > > On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <kh...@gmail.com>
> > > wrote:
> > >
> > > > Hi All,
> > > > I have raised a PR <
> > > https://github.com/apache/incubator-dubbo/pull/3027>for
> > > > issue 3026 <https://github.com/apache/incubator-dubbo/issues/3026>.
> In
> > > > this PR , I am reducing the number of SimpleDateFormat object
> creation
> > > > from each log message to per thread wise and reusing it. Please
> review
> > > it.
> > > >
> > > > https://github.com/apache/incubator-dubbo/pull/3027
> > > >
> > >
> >
>
Re: AccessLogFilter simple date format instance creation change PR
Posted by Imteyaz Khan <kh...@gmail.com>.
Due to my own confusion and bad I found I found my PR is containing more
commits and more file modification other than mine. So avoid the confusion
I created new PR and close the old one. This PR containing the all the
review comments from old PR. Please review mine this PR
<https://github.com/apache/incubator-dubbo/pull/3090>
https://github.com/apache/incubator-dubbo/pull/3090
Sorry for the inconvenience caused.
On Thu, Dec 27, 2018 at 3:24 PM yuhang xiu <ca...@gmail.com> wrote:
> Hi,
>
> I simply checked your pr and I agree with you. I will review it.
> Nice work!
>
> Imteyaz Khan <kh...@gmail.com> 于2018年12月27日周四 下午5:35写道：
>
> > Hi All,
> > I have raised a separate PR for the same issue 3026.
> >
> > https://github.com/apache/incubator-dubbo/pull/3080
> >
> > As my old PR was containing may commits of others and reviewer would like
> > me to clear those commits and I tried hard to achieve the same but failed
> > so landed up creating new PR
> > <https://github.com/apache/incubator-dubbo/pull/3080>.
> >
> > Please note :: older PR
> > <https://github.com/apache/incubator-dubbo/pull/3027> 3027 has been
> closed
> > but all the comments are applicable here.
> >
> > On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <kh...@gmail.com>
> > wrote:
> >
> > > Hi All,
> > > I have raised a PR <
> > https://github.com/apache/incubator-dubbo/pull/3027>for
> > > issue 3026 <https://github.com/apache/incubator-dubbo/issues/3026>. In
> > > this PR , I am reducing the number of SimpleDateFormat object creation
> > > from each log message to per thread wise and reusing it. Please review
> > it.
> > >
> > > https://github.com/apache/incubator-dubbo/pull/3027
> > >
> >
>
Re: AccessLogFilter simple date format instance creation change PR
Posted by yuhang xiu <ca...@gmail.com>.
Hi,
I simply checked your pr and I agree with you. I will review it.
Nice work!
Imteyaz Khan <kh...@gmail.com> 于2018年12月27日周四 下午5:35写道：
> Hi All,
> I have raised a separate PR for the same issue 3026.
>
> https://github.com/apache/incubator-dubbo/pull/3080
>
> As my old PR was containing may commits of others and reviewer would like
> me to clear those commits and I tried hard to achieve the same but failed
> so landed up creating new PR
> <https://github.com/apache/incubator-dubbo/pull/3080>.
>
> Please note :: older PR
> <https://github.com/apache/incubator-dubbo/pull/3027> 3027 has been closed
> but all the comments are applicable here.
>
> On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <kh...@gmail.com>
> wrote:
>
> > Hi All,
> > I have raised a PR <
> https://github.com/apache/incubator-dubbo/pull/3027>for
> > issue 3026 <https://github.com/apache/incubator-dubbo/issues/3026>. In
> > this PR , I am reducing the number of SimpleDateFormat object creation
> > from each log message to per thread wise and reusing it. Please review
> it.
> >
> > https://github.com/apache/incubator-dubbo/pull/3027
> >
>
Re: AccessLogFilter simple date format instance creation change PR
Posted by Imteyaz Khan <kh...@gmail.com>.
Hi All,
I have raised a separate PR for the same issue 3026.
https://github.com/apache/incubator-dubbo/pull/3080
As my old PR was containing may commits of others and reviewer would like
me to clear those commits and I tried hard to achieve the same but failed
so landed up creating new PR
<https://github.com/apache/incubator-dubbo/pull/3080>.
Please note :: older PR
<https://github.com/apache/incubator-dubbo/pull/3027> 3027 has been closed
but all the comments are applicable here.
On Thu, Dec 20, 2018 at 5:35 PM Imteyaz Khan <kh...@gmail.com> wrote:
> Hi All,
> I have raised a PR <https://github.com/apache/incubator-dubbo/pull/3027>for
> issue 3026 <https://github.com/apache/incubator-dubbo/issues/3026>. In
> this PR , I am reducing the number of SimpleDateFormat object creation
> from each log message to per thread wise and reusing it. Please review it.
>
> https://github.com/apache/incubator-dubbo/pull/3027
>