Hi Piotr,
Thanks for your feedback!
> Or implement your own loop? It shouldn't be more than a couple of lines.
Implementing it directly is fine, I have updated the FLIP.
And this logic can be found in the `isLineEnded` method.
Best,
Rui
On Thu, Nov 9, 2023 at 11:00 PM Piotr Nowojski <piotr.nowoj...@gmail.com>
wrote:
> Hi Rui,
>
> > I see java8 doesn't have `Arrays.equals(int[] a, int aFromIndex, int
> > aToIndex, int[] b, int bFromIndex, int bToIndex)`,
> > and java11 has it. Do you have any other suggestions for java8?
>
> Maybe use `ByteBuffer.wrap`?
>
> ByteBuffer.wrap(array, ..., ...).equals(ByteBuffer.wrap(array2, ..., ...))
>
> This shouldn't have overheads as far as I remember.
>
> Or implement your own loop? It shouldn't be more than a couple of lines.
>
> Best,
> Piotrek
>
> czw., 9 lis 2023 o 06:43 Rui Fan <1996fan...@gmail.com> napisał(a):
>
> > Hi Piotr, Archit, Feng and Hangxiang:
> >
> > Thanks a lot for your feedback!
> >
> > Following is my comment, please correct me if I misunderstood anything!
> >
> > To Piotr:
> >
> > > Is there a reason why you are suggesting to copy out bytes from `buf`
> to
> > `bytes`,
> > > instead of using `Arrays.equals(int[] a, int aFromIndex, int aToIndex,
> > int[] b, int bFromIndex, int bToIndex)`?
> >
> > I see java8 doesn't have `Arrays.equals(int[] a, int aFromIndex, int
> > aToIndex, int[] b, int bFromIndex, int bToIndex)`,
> > and java11 has it. Do you have any other suggestions for java8?
> >
> > Also, this code doesn't run in production. As the comment of
> > System.lineSeparator():
> >
> > > On UNIX systems, it returns {@code "\n"}; on Microsoft
> > > Windows systems it returns {@code "\r\n"}.
> >
> > So Mac and Linux just return one character, we will compare
> > one byte directly.
> >
> >
> >
> > To Feng:
> >
> > > Will they be written to the taskManager.log file by default
> > > or the taskManager.out file?
> >
> > I prefer LOG as the default value for taskmanager.system-out.mode.
> > It's useful for job stability and doesn't introduce significant impact to
> > users. Also, our production has already used this feature for
> > more than 1 years, it works well.
> >
> > However, I write the DEFAULT as the default value for
> > taskmanager.system-out.mode, because when the community introduces
> > new options, the default value often selects the original behavior.
> >
> > Looking forward to hear more thoughts from community about this
> > default value.
> >
> > > If we can make taskmanager.out splittable and rolling, would it be
> > > easier for users to use this feature?
> >
> > Making taskmanager.out splittable and rolling is a good choice!
> > I have some concerns about it:
> >
> > 1. Users may also want to use LOG.info in their code and just
> > accidentally use System.out.println. It is possible that they will
> > also find the logs directly in taskmanager.log.
> > 2. I'm not sure whether the rolling strategy is easy to implement.
> > If we do it, it's necessary to define a series of flink options similar
> > to log options, such as: fileMax(how many files should be retained),
> > fileSize(The max size each file), fileNamePatten (The suffix of file
> > name),
> > 3. Check the file size periodically: all logs are written by log plugin,
> > they can check the log file size after writing. However, System.out
> > are written directly. And flink must start a thread to check the latest
> > taskmanager.out size periodically. If it's too quick, most of job
> aren't
> > necessary. If it's too slow, the file size cannot be controlled
> properly.
> >
> > Redirect it to LOG.info may be a reasonable and easy choice.
> > The user didn't really want to log into taskmanager.out, it just
> > happened by accident.
> >
> >
> >
> > To Hangxiang:
> >
> > > 1. I have a similar concern as Feng. Will we redirect to another log
> file
> > > not taskManager.log ?
> >
> > Please see my last comment, thanks!
> >
> > > taskManager.log contains lots of important information like init log.
> It
> > > will be rolled quickly if we redirect out and error here.
> >
> > IIUC, this issue isn't caused by System.out, and it can happen if user
> > call a lot of LOG.info. As I mentioned before: the user didn't really
> want
> > to log into taskmanager.out, it just happened by accident.
> > So, if users change the System.out to LOG.info, it still happen.
> >
> > > 2. Since we have redirected to LOG mode, Could we also log the subtask
> > info
> > > ? It may help us to debug granularly.
> >
> > I'm not sure what `log the subtask info` means. Let me confirm with you
> > first.
> > Do you mean like this: LOG.info("taskName {} : {}", taskName,
> > userLogContext)?
> >
> > Best,
> > Rui
> >
> > On Thu, Nov 9, 2023 at 11:47 AM Hangxiang Yu <master...@gmail.com>
> wrote:
> >
> > > Hi, Rui.
> > > Thanks for the proposal. It sounds reasonable.
> > > I have some questions, PTAL:
> > > 1. I have a similar concern as Feng. Will we redirect to another log
> file
> > > not taskManager.log ?
> > > taskManager.log contains lots of important information like init log.
> It
> > > will be rolled quickly if we redirect out and error here.
> > > 2. Since we have redirected to LOG mode, Could we also log the subtask
> > info
> > > ? It may help us to debug granularly.
> > >
> > > On Thu, Nov 9, 2023 at 9:47 AM Feng Jin <jinfeng1...@gmail.com> wrote:
> > >
> > > > Hi, Rui.
> > > >
> > > > Thank you for initiating this proposal.
> > > >
> > > > I have a question regarding redirecting stdout and stderr to LOG:
> > > >
> > > > Will they be written to the taskManager.log file by default or the
> > > > taskManager.out file?
> > > > If we can make taskmanager.out splittable and rolling, would it be
> > easier
> > > > for users to use this feature?
> > > >
> > > > Best,
> > > > Feng
> > > >
> > > > On Thu, Nov 9, 2023 at 3:15 AM Archit Goyal
> > <argo...@linkedin.com.invalid
> > > >
> > > > wrote:
> > > >
> > > > > Hi Rui,
> > > > >
> > > > > Thanks for the proposal.
> > > > >
> > > > > The proposed solution of supporting System out and err to be
> > redirected
> > > > to
> > > > > LOG or discarded and introducing an enum and two options to manage
> > > this,
> > > > > seems reasonable.
> > > > >
> > > > > +1
> > > > >
> > > > > Thanks,
> > > > > Archit Goyal
> > > > >
> > > > >
> > > > > From: Piotr Nowojski <pnowoj...@apache.org>
> > > > > Date: Wednesday, November 8, 2023 at 7:38 AM
> > > > > To: dev@flink.apache.org <dev@flink.apache.org>
> > > > > Subject: Re: [DISCUSS] FLIP-390: Support System out and err to be
> > > > > redirected to LOG or discarded
> > > > > Hi Rui,
> > > > >
> > > > > Thanks for the proposal.
> > > > >
> > > > > +1 I don't have any major comments :)
> > > > >
> > > > > One nit. In `SystemOutRedirectToLog` in this code:
> > > > >
> > > > > System.arraycopy(buf, count - LINE_SEPARATOR_LENGTH,
> > bytes,
> > > 0,
> > > > > LINE_SEPARATOR_LENGTH);
> > > > > return Arrays.equals(LINE_SEPARATOR_BYTES, bytes)
> > > > >
> > > > > Is there a reason why you are suggesting to copy out bytes from
> `buf`
> > > to
> > > > > `bytes`,
> > > > > instead of using `Arrays.equals(int[] a, int aFromIndex, int
> > aToIndex,
> > > > > int[] b, int bFromIndex, int bToIndex)`?
> > > > >
> > > > > Best,
> > > > > Piotrek
> > > > >
> > > > > śr., 8 lis 2023 o 11:53 Rui Fan <1996fan...@gmail.com> napisał(a):
> > > > >
> > > > > > Hi all!
> > > > > >
> > > > > > I would like to start a discussion of FLIP-390: Support System
> out
> > > and
> > > > > err
> > > > > > to be redirected to LOG or discarded[1].
> > > > > >
> > > > > > In various production environments, either cloud native or
> physical
> > > > > > machines, the disk space that Flink TaskManager can use is
> limited.
> > > > > >
> > > > > > In general, the flink users shouldn't use the
> `System.out.println`
> > in
> > > > > > production,
> > > > > > however this may happen when the number of Flink jobs and job
> > > > developers
> > > > > > is very large. Flink job may use System.out to output a large
> > amount
> > > of
> > > > > > data
> > > > > > to the taskmanager.out file. This file will not roll, it will
> > always
> > > > > > increment.
> > > > > > Eventually the upper limit of what the TM can be used for is
> > reached.
> > > > > >
> > > > > > We can support System out and err to be redirected to LOG or
> > > discarded,
> > > > > > the LOG can roll and won't increment forever.
> > > > > >
> > > > > > This feature is useful for SREs who maintain Flink environments,
> > they
> > > > can
> > > > > > redirect System.out to LOG by default. Although the cause of this
> > > > problem
> > > > > > is
> > > > > > that the user's code is not standardized, for SRE, pushing users
> to
> > > > > modify
> > > > > > the code one by one is usually a very time-consuming operation.
> > It's
> > > > also
> > > > > > useful for job stability where System.out is accidentally used.
> > > > > >
> > > > > > Looking forward to your feedback, thanks~
> > > > > >
> > > > > > [1]
> > > > >
> > > >
> > >
> >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fx%2F4guZE&data=05%7C01%7Cargoyal%40linkedin.com%7C937821de7bd846e6b97408dbe070beae%7C72f988bf86f141af91ab2d7cd011db47%7C0%7C0%7C638350547072823674%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zEv6B0Xiq2SNuU6Fm%2BAXnH%2BRILbm6Q0uDRbN7h6iaPM%3D&reserved=0
> > > > > <https://cwiki.apache.org/confluence/x/4guZE>
> > > > > >
> > > > > > Best,
> > > > > > Rui
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best,
> > > Hangxiang.
> > >
> >
>
