You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Han Li <li...@apache.org> on 2023/05/02 08:30:17 UTC
Re: [tomcat] branch main updated: Polish
On Apr 30, 2023, at 23:37, Mark Thomas <ma...@apache.org> wrote:
On 28/04/2023 03:58, lihan@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
lihan pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new fa0b2b196d Polish
fa0b2b196d is described below
commit fa0b2b196d8525a662e9edec258650865da465ed
Author: lihan <li...@apache.org>
AuthorDate: Fri Apr 28 10:57:48 2023 +0800
Polish
---
java/org/apache/catalina/core/ContainerBase.java | 18 +++++++--------
java/org/apache/catalina/core/StandardServer.java | 26
+++++++++-------------
.../apache/catalina/valves/JsonAccessLogValve.java | 2 +-
3 files changed, 21 insertions(+), 25 deletions(-)
diff --git a/java/org/apache/catalina/core/ContainerBase.java
b/java/org/apache/catalina/core/ContainerBase.java
index 9dc018be15..784c9032ef 100644
--- a/java/org/apache/catalina/core/ContainerBase.java
+++ b/java/org/apache/catalina/core/ContainerBase.java
@@ -377,7 +377,7 @@ public abstract class ContainerBase extends
LifecycleMBeanBase implements Contai
this.cluster = cluster;
// Stop the old component if necessary
- if (getState().isAvailable() && (oldCluster != null) && (oldCluster
instanceof Lifecycle)) {
+ if (getState().isAvailable() && (oldCluster instanceof Lifecycle)) {
try {
((Lifecycle) oldCluster).stop();
} catch (LifecycleException e) {
@@ -390,7 +390,7 @@ public abstract class ContainerBase extends
LifecycleMBeanBase implements Contai
cluster.setContainer(this);
}
- if (getState().isAvailable() && (cluster != null) && (cluster instanceof
Lifecycle)) {
+ if (getState().isAvailable() && (cluster instanceof Lifecycle)) {
try {
((Lifecycle) cluster).start();
} catch (LifecycleException e) {
@@ -580,7 +580,7 @@ public abstract class ContainerBase extends
LifecycleMBeanBase implements Contai
this.realm = realm;
// Stop the old component if necessary
- if (getState().isAvailable() && (oldRealm != null) && (oldRealm
instanceof Lifecycle)) {
+ if (getState().isAvailable() && (oldRealm instanceof Lifecycle)) {
try {
((Lifecycle) oldRealm).stop();
} catch (LifecycleException e) {
@@ -592,7 +592,7 @@ public abstract class ContainerBase extends
LifecycleMBeanBase implements Contai
if (realm != null) {
realm.setContainer(this);
}
- if (getState().isAvailable() && (realm != null) && (realm instanceof
Lifecycle)) {
+ if (getState().isAvailable() && (realm instanceof Lifecycle)) {
try {
((Lifecycle) realm).start();
} catch (LifecycleException e) {
@@ -832,8 +832,8 @@ public abstract class ContainerBase extends
LifecycleMBeanBase implements Contai
}
// Start our child containers, if any
- Container children[] = findChildren();
- List<Future<Void>> results = new ArrayList<>();
+ Container[] children = findChildren();
+ List<Future<Void>> results = new ArrayList<>(children.length);
for (Container child : children) {
results.add(startStopExecutor.submit(new StartChild(child)));
}
@@ -897,8 +897,8 @@ public abstract class ContainerBase extends
LifecycleMBeanBase implements Contai
}
// Stop our child containers, if any
- Container children[] = findChildren();
- List<Future<Void>> results = new ArrayList<>();
+ Container[] children = findChildren();
+ List<Future<Void>> results = new ArrayList<>(children.length);
for (Container child : children) {
results.add(startStopExecutor.submit(new StopChild(child)));
}
@@ -992,7 +992,7 @@ public abstract class ContainerBase extends
LifecycleMBeanBase implements Contai
}
AccessLogAdapter adapter = null;
- Valve valves[] = getPipeline().getValves();
+ Valve[] valves = getPipeline().getValves();
for (Valve valve : valves) {
if (valve instanceof AccessLog) {
if (adapter == null) {
diff --git a/java/org/apache/catalina/core/StandardServer.java
b/java/org/apache/catalina/core/StandardServer.java
index eb5e91e932..09a223fa80 100644
--- a/java/org/apache/catalina/core/StandardServer.java
+++ b/java/org/apache/catalina/core/StandardServer.java
@@ -135,7 +135,7 @@ public final class StandardServer extends
LifecycleMBeanBase implements Server {
/**
* The set of Services associated with this Server.
*/
- private Service services[] = new Service[0];
+ private Service[] services = new Service[0];
private final Object servicesLock = new Object();
@@ -175,12 +175,12 @@ public final class StandardServer extends
LifecycleMBeanBase implements Server {
/**
* The number of threads available to process utility tasks in this service.
*/
- protected int utilityThreads = 2;
+ private int utilityThreads = 2;
This is changing the public API. We can do that in 11.0.x but not in
earlier versions. If we do make this change in 11.0.x then we really should
deprecated the field in at least 10.1.x to provide users with advance
warning of the change.
Sorry Mark, I am so confusing that why this called changed the public API.
According my review, users can not access this field directly with Server
instance as the modifier is protected and whom can not extend
StandardServer to do same operation as the modifier of StandardServer is
final. So only us can access this field directly in StandardServer, no? Or
maybe I miss something? : (
The same comment applies to all the fields changed from protected to
private.
To be clear, I am in favour of this change. I'd prefer us to be using
public getters and setters rather than protected fields. But any such
change needs to be communicated to the users.
+1
Thanks
Han
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch main updated: Polish
Posted by koteswara Rao Gundapaneni <ko...@gmail.com>.
Hi Team,
private int utilityThreads = 2; should be ideal that if its 3 keep it in
understanding/view of BinaryTree (Heap Memory) so that the server make full
advantage.
Any comments are more accepted if those are not missed by the mailinglist
Regards,
Koti
On Tue, May 2, 2023 at 1:41 AM Mark Thomas <ma...@apache.org> wrote:
> On 02/05/2023 09:30, Han Li wrote:
> > On Apr 30, 2023, at 23:37, Mark Thomas <ma...@apache.org> wrote:
> >
> > On 28/04/2023 03:58, lihan@apache.org wrote:
> >
> > This is an automated email from the ASF dual-hosted git repository.
> > lihan pushed a commit to branch main
> > in repository https://gitbox.apache.org/repos/asf/tomcat.git
> > The following commit(s) were added to refs/heads/main by this push:
> > new fa0b2b196d Polish
> > fa0b2b196d is described below
> > commit fa0b2b196d8525a662e9edec258650865da465ed
> > Author: lihan <li...@apache.org>
> > AuthorDate: Fri Apr 28 10:57:48 2023 +0800
> > Polish
> > ---
> > java/org/apache/catalina/core/ContainerBase.java | 18 +++++++--------
> > java/org/apache/catalina/core/StandardServer.java | 26
> > +++++++++-------------
> > .../apache/catalina/valves/JsonAccessLogValve.java | 2 +-
> > 3 files changed, 21 insertions(+), 25 deletions(-)
> > diff --git a/java/org/apache/catalina/core/ContainerBase.java
> > b/java/org/apache/catalina/core/ContainerBase.java
> > index 9dc018be15..784c9032ef 100644
> > --- a/java/org/apache/catalina/core/ContainerBase.java
> > +++ b/java/org/apache/catalina/core/ContainerBase.java
> > @@ -377,7 +377,7 @@ public abstract class ContainerBase extends
> > LifecycleMBeanBase implements Contai
> > this.cluster = cluster;
> > // Stop the old component if necessary
> > - if (getState().isAvailable() && (oldCluster != null) && (oldCluster
> > instanceof Lifecycle)) {
> > + if (getState().isAvailable() && (oldCluster instanceof Lifecycle)) {
> > try {
> > ((Lifecycle) oldCluster).stop();
> > } catch (LifecycleException e) {
> > @@ -390,7 +390,7 @@ public abstract class ContainerBase extends
> > LifecycleMBeanBase implements Contai
> > cluster.setContainer(this);
> > }
> > - if (getState().isAvailable() && (cluster != null) && (cluster
> instanceof
> > Lifecycle)) {
> > + if (getState().isAvailable() && (cluster instanceof Lifecycle)) {
> > try {
> > ((Lifecycle) cluster).start();
> > } catch (LifecycleException e) {
> > @@ -580,7 +580,7 @@ public abstract class ContainerBase extends
> > LifecycleMBeanBase implements Contai
> > this.realm = realm;
> > // Stop the old component if necessary
> > - if (getState().isAvailable() && (oldRealm != null) && (oldRealm
> > instanceof Lifecycle)) {
> > + if (getState().isAvailable() && (oldRealm instanceof Lifecycle)) {
> > try {
> > ((Lifecycle) oldRealm).stop();
> > } catch (LifecycleException e) {
> > @@ -592,7 +592,7 @@ public abstract class ContainerBase extends
> > LifecycleMBeanBase implements Contai
> > if (realm != null) {
> > realm.setContainer(this);
> > }
> > - if (getState().isAvailable() && (realm != null) && (realm instanceof
> > Lifecycle)) {
> > + if (getState().isAvailable() && (realm instanceof Lifecycle)) {
> > try {
> > ((Lifecycle) realm).start();
> > } catch (LifecycleException e) {
> > @@ -832,8 +832,8 @@ public abstract class ContainerBase extends
> > LifecycleMBeanBase implements Contai
> > }
> > // Start our child containers, if any
> > - Container children[] = findChildren();
> > - List<Future<Void>> results = new ArrayList<>();
> > + Container[] children = findChildren();
> > + List<Future<Void>> results = new ArrayList<>(children.length);
> > for (Container child : children) {
> > results.add(startStopExecutor.submit(new StartChild(child)));
> > }
> > @@ -897,8 +897,8 @@ public abstract class ContainerBase extends
> > LifecycleMBeanBase implements Contai
> > }
> > // Stop our child containers, if any
> > - Container children[] = findChildren();
> > - List<Future<Void>> results = new ArrayList<>();
> > + Container[] children = findChildren();
> > + List<Future<Void>> results = new ArrayList<>(children.length);
> > for (Container child : children) {
> > results.add(startStopExecutor.submit(new StopChild(child)));
> > }
> > @@ -992,7 +992,7 @@ public abstract class ContainerBase extends
> > LifecycleMBeanBase implements Contai
> > }
> > AccessLogAdapter adapter = null;
> > - Valve valves[] = getPipeline().getValves();
> > + Valve[] valves = getPipeline().getValves();
> > for (Valve valve : valves) {
> > if (valve instanceof AccessLog) {
> > if (adapter == null) {
> > diff --git a/java/org/apache/catalina/core/StandardServer.java
> > b/java/org/apache/catalina/core/StandardServer.java
> > index eb5e91e932..09a223fa80 100644
> > --- a/java/org/apache/catalina/core/StandardServer.java
> > +++ b/java/org/apache/catalina/core/StandardServer.java
> > @@ -135,7 +135,7 @@ public final class StandardServer extends
> > LifecycleMBeanBase implements Server {
> > /**
> > * The set of Services associated with this Server.
> > */
> > - private Service services[] = new Service[0];
> > + private Service[] services = new Service[0];
> > private final Object servicesLock = new Object();
> > @@ -175,12 +175,12 @@ public final class StandardServer extends
> > LifecycleMBeanBase implements Server {
> > /**
> > * The number of threads available to process utility tasks in this
> service.
> > */
> > - protected int utilityThreads = 2;
> > + private int utilityThreads = 2;
> >
> >
> > This is changing the public API. We can do that in 11.0.x but not in
> > earlier versions. If we do make this change in 11.0.x then we really
> should
> > deprecated the field in at least 10.1.x to provide users with advance
> > warning of the change.
> >
> >
> > Sorry Mark, I am so confusing that why this called changed the public
> API.
> > According my review, users can not access this field directly with Server
> > instance as the modifier is protected and whom can not extend
> > StandardServer to do same operation as the modifier of StandardServer is
> > final. So only us can access this field directly in StandardServer, no?
> Or
> > maybe I miss something? : (
>
> No, you are right and it is me that missed something. Sorry. I didn't
> notice StandardServer was final.
>
> Mark
>
>
> >
> >
> > The same comment applies to all the fields changed from protected to
> > private.
> >
> > To be clear, I am in favour of this change. I'd prefer us to be using
> > public getters and setters rather than protected fields. But any such
> > change needs to be communicated to the users.
> >
> > +1
> >
> >
> > Thanks
> >
> > Han
> >
> >
> > Mark
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: [tomcat] branch main updated: Polish
Posted by Mark Thomas <ma...@apache.org>.
On 02/05/2023 09:30, Han Li wrote:
> On Apr 30, 2023, at 23:37, Mark Thomas <ma...@apache.org> wrote:
>
> On 28/04/2023 03:58, lihan@apache.org wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
> lihan pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
> The following commit(s) were added to refs/heads/main by this push:
> new fa0b2b196d Polish
> fa0b2b196d is described below
> commit fa0b2b196d8525a662e9edec258650865da465ed
> Author: lihan <li...@apache.org>
> AuthorDate: Fri Apr 28 10:57:48 2023 +0800
> Polish
> ---
> java/org/apache/catalina/core/ContainerBase.java | 18 +++++++--------
> java/org/apache/catalina/core/StandardServer.java | 26
> +++++++++-------------
> .../apache/catalina/valves/JsonAccessLogValve.java | 2 +-
> 3 files changed, 21 insertions(+), 25 deletions(-)
> diff --git a/java/org/apache/catalina/core/ContainerBase.java
> b/java/org/apache/catalina/core/ContainerBase.java
> index 9dc018be15..784c9032ef 100644
> --- a/java/org/apache/catalina/core/ContainerBase.java
> +++ b/java/org/apache/catalina/core/ContainerBase.java
> @@ -377,7 +377,7 @@ public abstract class ContainerBase extends
> LifecycleMBeanBase implements Contai
> this.cluster = cluster;
> // Stop the old component if necessary
> - if (getState().isAvailable() && (oldCluster != null) && (oldCluster
> instanceof Lifecycle)) {
> + if (getState().isAvailable() && (oldCluster instanceof Lifecycle)) {
> try {
> ((Lifecycle) oldCluster).stop();
> } catch (LifecycleException e) {
> @@ -390,7 +390,7 @@ public abstract class ContainerBase extends
> LifecycleMBeanBase implements Contai
> cluster.setContainer(this);
> }
> - if (getState().isAvailable() && (cluster != null) && (cluster instanceof
> Lifecycle)) {
> + if (getState().isAvailable() && (cluster instanceof Lifecycle)) {
> try {
> ((Lifecycle) cluster).start();
> } catch (LifecycleException e) {
> @@ -580,7 +580,7 @@ public abstract class ContainerBase extends
> LifecycleMBeanBase implements Contai
> this.realm = realm;
> // Stop the old component if necessary
> - if (getState().isAvailable() && (oldRealm != null) && (oldRealm
> instanceof Lifecycle)) {
> + if (getState().isAvailable() && (oldRealm instanceof Lifecycle)) {
> try {
> ((Lifecycle) oldRealm).stop();
> } catch (LifecycleException e) {
> @@ -592,7 +592,7 @@ public abstract class ContainerBase extends
> LifecycleMBeanBase implements Contai
> if (realm != null) {
> realm.setContainer(this);
> }
> - if (getState().isAvailable() && (realm != null) && (realm instanceof
> Lifecycle)) {
> + if (getState().isAvailable() && (realm instanceof Lifecycle)) {
> try {
> ((Lifecycle) realm).start();
> } catch (LifecycleException e) {
> @@ -832,8 +832,8 @@ public abstract class ContainerBase extends
> LifecycleMBeanBase implements Contai
> }
> // Start our child containers, if any
> - Container children[] = findChildren();
> - List<Future<Void>> results = new ArrayList<>();
> + Container[] children = findChildren();
> + List<Future<Void>> results = new ArrayList<>(children.length);
> for (Container child : children) {
> results.add(startStopExecutor.submit(new StartChild(child)));
> }
> @@ -897,8 +897,8 @@ public abstract class ContainerBase extends
> LifecycleMBeanBase implements Contai
> }
> // Stop our child containers, if any
> - Container children[] = findChildren();
> - List<Future<Void>> results = new ArrayList<>();
> + Container[] children = findChildren();
> + List<Future<Void>> results = new ArrayList<>(children.length);
> for (Container child : children) {
> results.add(startStopExecutor.submit(new StopChild(child)));
> }
> @@ -992,7 +992,7 @@ public abstract class ContainerBase extends
> LifecycleMBeanBase implements Contai
> }
> AccessLogAdapter adapter = null;
> - Valve valves[] = getPipeline().getValves();
> + Valve[] valves = getPipeline().getValves();
> for (Valve valve : valves) {
> if (valve instanceof AccessLog) {
> if (adapter == null) {
> diff --git a/java/org/apache/catalina/core/StandardServer.java
> b/java/org/apache/catalina/core/StandardServer.java
> index eb5e91e932..09a223fa80 100644
> --- a/java/org/apache/catalina/core/StandardServer.java
> +++ b/java/org/apache/catalina/core/StandardServer.java
> @@ -135,7 +135,7 @@ public final class StandardServer extends
> LifecycleMBeanBase implements Server {
> /**
> * The set of Services associated with this Server.
> */
> - private Service services[] = new Service[0];
> + private Service[] services = new Service[0];
> private final Object servicesLock = new Object();
> @@ -175,12 +175,12 @@ public final class StandardServer extends
> LifecycleMBeanBase implements Server {
> /**
> * The number of threads available to process utility tasks in this service.
> */
> - protected int utilityThreads = 2;
> + private int utilityThreads = 2;
>
>
> This is changing the public API. We can do that in 11.0.x but not in
> earlier versions. If we do make this change in 11.0.x then we really should
> deprecated the field in at least 10.1.x to provide users with advance
> warning of the change.
>
>
> Sorry Mark, I am so confusing that why this called changed the public API.
> According my review, users can not access this field directly with Server
> instance as the modifier is protected and whom can not extend
> StandardServer to do same operation as the modifier of StandardServer is
> final. So only us can access this field directly in StandardServer, no? Or
> maybe I miss something? : (
No, you are right and it is me that missed something. Sorry. I didn't
notice StandardServer was final.
Mark
>
>
> The same comment applies to all the fields changed from protected to
> private.
>
> To be clear, I am in favour of this change. I'd prefer us to be using
> public getters and setters rather than protected fields. But any such
> change needs to be communicated to the users.
>
> +1
>
>
> Thanks
>
> Han
>
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org