Re: Geode-9621: Spotless overstepping its boundary of formatter

[Udo Kohlmeyer]

Hi there Robert,

Yes, you did work with me, but the solution to add my custom versions of jars 
that I want to test with, into the `DependencyConstraints` file just feels like 
the wrong approach. The prior art you refer to, is a specific version of  
tomcat we want each module to use. It is no different to specifying the exact 
artifact version we want for production code. My case is that I don’t want my 
artifact version dependencies to leak outside of the test module.

I think, the fact that the decision to elevate spotless from cosmetic formatter 
to actually having a significant impact, was done unilaterally. Please don’t 
get me wrong, I love the fact that we want to fix the problem where developers 
are not following good practices. But I think I am digging in here, because I 
believe we missed an opportunity to get more voices in this decision.

I also firmly believe that spotless is overstepping the “cosmetic” boundary. I 
think many developers would feel upset if their code were to be changed. If 
spotless suddenly replaces a for-loop with a stream implementation.

I think it is one thing to decide if gradle should be formatted with 2 spaces 
vs 4, or when and how lines wrap, and it’s another for spotless to just remove 
version information and force one consistent version even if the version was 
specifically overriden.

I wonder if we can maybe reduce the area of effect of this change and really 
only force the version formatter to activate for the ‘main’ configuration and 
leave every other configuration type (test, distributedTest, integrationTest, 
acceptanceTest, upgradeTest) alone.

--Udo

From: Robert Houghton 
Date: Tuesday, October 12, 2021 at 10:38 AM
To: dev@geode.apache.org 
Subject: Re: Geode-9621: Spotless overstepping its boundary of formatter
Currently there is no way to exclude certain code blocks from the malicious 
code formatter. Therefore, my tests that require a specific hardcoded version 
of Spring to be used, will always either fail the validation or tests will fail 
due to spotless changing my testing scenario by affecting the version of the 
library that we have to use.


Udo,

I have worked with you to accomplish your goal of not automatically “fixing” 
the inclusion of dependency versions in the build files, as your work case 
wants to do something special there. The formatter now throws when there is a 
naked version-string on the dependency line, as we discussed.

If you want to continue to work around this, there are many cases of prior art 
in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific 
versions are allowed.

Good luck.
-Robert Houghton

Re: Geode-9621: Spotless overstepping its boundary of formatter

[Robert Houghton]

Currently there is no way to exclude certain code blocks from the malicious 
code formatter. Therefore, my tests that require a specific hardcoded version 
of Spring to be used, will always either fail the validation or tests will fail 
due to spotless changing my testing scenario by affecting the version of the 
library that we have to use.


Udo,

I have worked with you to accomplish your goal of not automatically “fixing” 
the inclusion of dependency versions in the build files, as your work case 
wants to do something special there. The formatter now throws when there is a 
naked version-string on the dependency line, as we discussed.

If you want to continue to work around this, there are many cases of prior art 
in Geode, such as extensions/geode-modules-tomcat9/build.gradle, where specific 
versions are allowed.

Good luck.
-Robert Houghton

Geode-9621: Spotless overstepping its boundary of formatter

[Udo Kohlmeyer]

Hi there Geode Devs,

Recently 
GEODE-9621 was 
introduced onto the develop branch. This new feature introduces a new bug where 
`spotlessApply` will remove all version dependencies out of gradle. I assume 
this is to combat the rising number of cases where developers have been 
hard-coding versions into the gradle files, rather than using the constraints 
resolution.

Whilst I like this feature, it also has its draw backs. There is no way to turn 
off this feature WHEN version overrides are required. An example would be a 
test where one wants to specifically test a specific version of a library. In 
my case, this is Spring 4.3.5. My test is designed to deploy a version of 
Spring, onto Geode, and make sure that the versions of the libraries don’t 
conflict with one another.

We’ve all agreed to spotless when the code has not conformed to the coding 
standards that we have all agreed to. To automagically apply the code 
formatting, using `spotlessApply` is an accepted norm, which each of us follow. 
But the one thing that I cannot accept, and I hope many of you could not do 
this either, is when spotless where to actually CHANGE our code. Apply some 
benign rule that could negatively impact the code behavior.

When we voted for the use of spotless, it was as a code formatter. To make sure 
that our code base has the same formatting. Seemingly, with GEODE-9621, 
spotless now have the ability to change the code. I can accept that gradle 
files will be formatted in specific manner, but I cannot accept that the 
formatter now has to the power to negatively impact behavior.

Currently there is no way to exclude certain code blocks from the malicious 
code formatter. Therefore, my tests that require a specific hardcoded version 
of Spring to be used, will always either fail the validation or tests will fail 
due to spotless changing my testing scenario by affecting the version of the 
library that we have to use.

Does anyone have any suggestions as to how we can fix this. My vote is to 
remove this behavior and limit spotless to only formatting. BUT, that is only 
my view.

--Udo

Re: Region is not created on one of the servers

[Udo Kohlmeyer]

Hi Mario,

I think your assessment of the problem is correct. Thinking about it, there is 
no simple (correct) way to easily solve this. Given that there are too many 
variables in play, users making configurational changes, whilst servers are 
coming up.

Now, that said, I think we should address this problem. I also think your 
assessment is correct that cluster configuration was not written to handle this 
scenario. I think some thought has to go into the algorithm that one would like 
to follow and how we would like to resolve it.

Can you please raise a ticket on this issue.

--Udo

From: Mario Kevo 
Date: Monday, October 11, 2021 at 11:27 PM
To: dev@geode.apache.org 
Subject: Odg: Region is not created on one of the servers
I think that there can be a problem if we change to first add it to cluster 
config and then do distribution to existing servers.

Now, when the "create region" command is executed it got all servers from the 
view and sends all of them to start creating a region with parameters specified 
in the command.
The region creating is started on all servers and after it is finished, it is 
added to the cluster configuration. In case there are some problems with 
creating a region(wrong parameter used or something else) it will not create a 
region on the existing servers and will not write anything in a cluster 
configuration.

In case we decide to change order, it will write in the cluster config before 
the command is successful, and then we should have some backup to rollback 
cluster configuration.

Also, this can happen for all commands that editing cluster configuration.

It looks like this is not designed to execute some commands in parallel with 
starting servers.

BR,
Mario

Šalje: Dan Smith 
Poslano: 8. listopada 2021. 20:37
Prima: dev@geode.apache.org 
Predmet: Re: Region is not created on one of the servers

This seems like something ought to work, so I would call it a bug if the region 
didn't get created on 1 server. At first glance, it looks like the problem is 
that we distribute the region to all the servers before adding it to cluster 
config? Seems like we need to do distribution after​ adding the region to 
cluster config, to make sure that all servers get the region.

-Dan

From: Mario Kevo 
Sent: Friday, October 8, 2021 5:31 AM
To: dev@geode.apache.org 
Subject: Region is not created on one of the servers

Hi geode-dev,

We are using a system with a large number of servers.
While starting all servers, in parallel, we create a region through gfsh.
The problem is that on one of the servers region is not created.

There is an example of the problem:

We started the locator, and then go with starting the servers, one by one.
In the meantime, we run the "create region" command through gfsh.
All servers that are started before the "create region" command got information 
to create a region on itself, but the problem is in the server which is started 
after the "create region" command is started and not finished yet.
After the "create region" command is finished, all other servers started after 
that will get that region in the cluster configuration and create it.

What happened with this one server without a region?
It is started after the "create region" command is started, so it will not get 
information to create a region on itself from the locator. Also, the cluster 
configuration doesn't have that information yet, so the server cannot read it 
from the received cluster configuration.

So the question is, is it allowed to run commands in parallel?
If yes, we should do some checks in the code to avoid this issue.
If not, we need to write it somewhere in the documentation.

BR,
Mario

Odg: Region is not created on one of the servers

[Mario Kevo]

I think that there can be a problem if we change to first add it to cluster 
config and then do distribution to existing servers.

Now, when the "create region" command is executed it got all servers from the 
view and sends all of them to start creating a region with parameters specified 
in the command.
The region creating is started on all servers and after it is finished, it is 
added to the cluster configuration. In case there are some problems with 
creating a region(wrong parameter used or something else) it will not create a 
region on the existing servers and will not write anything in a cluster 
configuration.

In case we decide to change order, it will write in the cluster config before 
the command is successful, and then we should have some backup to rollback 
cluster configuration.

Also, this can happen for all commands that editing cluster configuration.

It looks like this is not designed to execute some commands in parallel with 
starting servers.

BR,
Mario

Šalje: Dan Smith 
Poslano: 8. listopada 2021. 20:37
Prima: dev@geode.apache.org 
Predmet: Re: Region is not created on one of the servers

This seems like something ought to work, so I would call it a bug if the region 
didn't get created on 1 server. At first glance, it looks like the problem is 
that we distribute the region to all the servers before adding it to cluster 
config? Seems like we need to do distribution after​ adding the region to 
cluster config, to make sure that all servers get the region.

-Dan

From: Mario Kevo 
Sent: Friday, October 8, 2021 5:31 AM
To: dev@geode.apache.org 
Subject: Region is not created on one of the servers

Hi geode-dev,

We are using a system with a large number of servers.
While starting all servers, in parallel, we create a region through gfsh.
The problem is that on one of the servers region is not created.

There is an example of the problem:

We started the locator, and then go with starting the servers, one by one.
In the meantime, we run the "create region" command through gfsh.
All servers that are started before the "create region" command got information 
to create a region on itself, but the problem is in the server which is started 
after the "create region" command is started and not finished yet.
After the "create region" command is finished, all other servers started after 
that will get that region in the cluster configuration and create it.

What happened with this one server without a region?
It is started after the "create region" command is started, so it will not get 
information to create a region on itself from the locator. Also, the cluster 
configuration doesn't have that information yet, so the server cannot read it 
from the received cluster configuration.

So the question is, is it allowed to run commands in parallel?
If yes, we should do some checks in the code to avoid this issue.
If not, we need to write it somewhere in the documentation.

BR,
Mario