Refactoring of IgniteKernal ack methods

[Ivan Fedotov]

Hello, Igniters!

I found, that in several places of IgniteKernal class code blocks are huge
and hard to understand and in other places methods have the same context
and could be placed in their own class. For example methods:
“ackAsciiLogo”, “ackConfigUrl”, “ackDaemon”, “ackOsInfo”,
“ackLanguageRuntime”, “ackRemoteManagement”, “ackVmArguments”,
“ackClassPaths”, “ackSystemProperties”, “ackEnviromentVariables”,
“ackMemoryConfiguration”, “ackCacheConfiguration”, “ackP2PConfiguration”,
“ackRebalanceConfiguration”, which are called in 800-813 lines and together
contain over than 250 lines, can be placed in separate class
“AckVariousInformation”.

Do you think that it will be good to create task on such refactoring?


--
Ivan Fedotov.

[hidden email]

[dsetrakyan]

On Wed, Oct 11, 2017 at 7:44 AM, Иван Федотов <[hidden email]> wrote:

> Hello, Igniters!
>
> I found, that in several places of IgniteKernal class code blocks are huge
> and hard to understand and in other places methods have the same context
> and could be placed in their own class. For example methods:
> “ackAsciiLogo”, “ackConfigUrl”, “ackDaemon”, “ackOsInfo”,
> “ackLanguageRuntime”, “ackRemoteManagement”, “ackVmArguments”,
> “ackClassPaths”, “ackSystemProperties”, “ackEnviromentVariables”,
> “ackMemoryConfiguration”, “ackCacheConfiguration”, “ackP2PConfiguration”,
> “ackRebalanceConfiguration”, which are called in 800-813 lines and together
> contain over than 250 lines, can be placed in separate class
> “AckVariousInformation”.
>
> Do you think that it will be good to create task on such refactoring?
>

I think this is a matter of taste. One could argue that the code is more
readable now because these methods belong to IgniteKernal and are not
called from any other place. I would generally such changes.

[Dmitrii Ryabov]

IgniteKernal and some other classes have more places to refactor. At least
for better readable form or to remove inner class [1]. May be create some
tickets for refactoring such complex places? I mean to create main ticket
like [2] or [3], and when someone see something complex *and* the way to
refactor - he can create subtask/PR for refactoring. Thoughts?

[1] - http://apache-ignite-developers.2346864.n4.nabble.com/Mini
mize-amount-of-inner-classes-predicates-tuples-etc-td17689.html
[2] - https://issues.apache.org/jira/browse/IGNITE-4575
[3] - https://issues.apache.org/jira/browse/IGNITE-5156


2017-10-11 18:37 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:

> On Wed, Oct 11, 2017 at 7:44 AM, Иван Федотов <[hidden email]> wrote:
>
> > Hello, Igniters!
> >
> > I found, that in several places of IgniteKernal class code blocks are
> huge
> > and hard to understand and in other places methods have the same context
> > and could be placed in their own class. For example methods:
> > “ackAsciiLogo”, “ackConfigUrl”, “ackDaemon”, “ackOsInfo”,
> > “ackLanguageRuntime”, “ackRemoteManagement”, “ackVmArguments”,
> > “ackClassPaths”, “ackSystemProperties”, “ackEnviromentVariables”,
> > “ackMemoryConfiguration”, “ackCacheConfiguration”, “ackP2PConfiguration”,
> > “ackRebalanceConfiguration”, which are called in 800-813 lines and
> together
> > contain over than 250 lines, can be placed in separate class
> > “AckVariousInformation”.
> >
> > Do you think that it will be good to create task on such refactoring?
> >
>
> I think this is a matter of taste. One could argue that the code is more
> readable now because these methods belong to IgniteKernal and are not
> called from any other place. I would generally such changes.
>

[Vladimir Ozerov]

Dmitry,

In my experience refactoring without a reasons often produces more harm
than benefits. Especially:
1) You may brake compatibility. E.g. moving anonymous inner class to the
top level will cause renames of all other anonymous classes.
2) This may interfere severely with other people's work. If someone works
on some issue in a separate branch, he will get a lot of severe conflicts
when trying to merge his branch with master.

So I do not think we should do refactoring for refactoring. We should do
this only for a strong reason.

On Thu, Oct 12, 2017 at 5:12 PM, Дмитрий Рябов <[hidden email]>
wrote:

> IgniteKernal and some other classes have more places to refactor. At least
> for better readable form or to remove inner class [1]. May be create some
> tickets for refactoring such complex places? I mean to create main ticket
> like [2] or [3], and when someone see something complex *and* the way to
> refactor - he can create subtask/PR for refactoring. Thoughts?
>
> [1] - http://apache-ignite-developers.2346864.n4.nabble.com/Mini
> mize-amount-of-inner-classes-predicates-tuples-etc-td17689.html
> [2] - https://issues.apache.org/jira/browse/IGNITE-4575
> [3] - https://issues.apache.org/jira/browse/IGNITE-5156
>
>
> 2017-10-11 18:37 GMT+03:00 Dmitriy Setrakyan <[hidden email]>:
>
> > On Wed, Oct 11, 2017 at 7:44 AM, Иван Федотов <[hidden email]>
> wrote:
> >
> > > Hello, Igniters!
> > >
> > > I found, that in several places of IgniteKernal class code blocks are
> > huge
> > > and hard to understand and in other places methods have the same
> context
> > > and could be placed in their own class. For example methods:
> > > “ackAsciiLogo”, “ackConfigUrl”, “ackDaemon”, “ackOsInfo”,
> > > “ackLanguageRuntime”, “ackRemoteManagement”, “ackVmArguments”,
> > > “ackClassPaths”, “ackSystemProperties”, “ackEnviromentVariables”,
> > > “ackMemoryConfiguration”, “ackCacheConfiguration”,
> “ackP2PConfiguration”,
> > > “ackRebalanceConfiguration”, which are called in 800-813 lines and
> > together
> > > contain over than 250 lines, can be placed in separate class
> > > “AckVariousInformation”.
> > >
> > > Do you think that it will be good to create task on such refactoring?
> > >
> >
> > I think this is a matter of taste. One could argue that the code is more
> > readable now because these methods belong to IgniteKernal and are not
> > called from any other place. I would generally such changes.
> >
>