About MINA commits with some code refactoring
Hi guys, recently I saw some commits on MINA where some code has been refactored (like lines were limited to 80 caracters, stuff like that). I don't think it's a bad thing to do code refactoring, but the problem is that when it's mixed with some code addition, it buries the fixes into a lot of refactoring, and it's difficult for reviewers to check the fixes. My personal guess is that those who are committing such code are using some specific options on their favorite IDE, leading to such injection of code. It's not only about line splitting, it can be addition of windows style EOL, or even tabs. I'm not free from such injections, I must admit. There is a Eclipse formating file for those who are using Eclipse, that must be used in order to avoid such automatic refactoring : http://mina.apache.org/developer-guide.html#DeveloperGuide-CodingConvention If you are using another IDE, it could be cool to create the same kind of file. Last, not least, if you think the file need to be refactored, feel free to do so, but *please*, do that in a separate commit : first refactor the code, commit it and then fix the code and commit again. Many thanks ! -- -- cordialement, regards, Emmanuel Lécharny www.iktek.com directory.apache.org ,
Re: About MINA commits with some code refactoring
Le Mon, 29 Jun 2009 11:08:33 +0200, Emmanuel Lecharny elecha...@apache.org a écrit : Hi guys, recently I saw some commits on MINA where some code has been refactored (like lines were limited to 80 caracters, stuff like that). I don't think it's a bad thing to do code refactoring, but the problem is that when it's mixed with some code addition, it buries the fixes into a lot of refactoring, and it's difficult for reviewers to check the fixes. My personal guess is that those who are committing such code are using some specific options on their favorite IDE, leading to such injection of code. It's not only about line splitting, it can be addition of windows style EOL, or even tabs. I'm not free from such injections, I must admit. There is a Eclipse formating file for those who are using Eclipse, that must be used in order to avoid such automatic refactoring : http://mina.apache.org/developer-guide.html#DeveloperGuide-CodingConvention If you are using another IDE, it could be cool to create the same kind of file. Last, not least, if you think the file need to be refactored, feel free to do so, but *please*, do that in a separate commit : first refactor the code, commit it and then fix the code and commit again. Many thanks ! +1 It's painful to decipher some commits. Julien signature.asc Description: PGP signature
Re: svn commit: r789164 - /mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
Hi,
Interesting optimization. The idea is to make SimpleIoPPool
instantiation faster ?
Julien
On Mon, Jun 29, 2009 at 12:30 AM, edeolive...@apache.org wrote:
Author: edeoliveira
Date: Sun Jun 28 22:30:02 2009
New Revision: 789164
URL: http://svn.apache.org/viewvc?rev=789164view=rev
Log:
Fix to do less reflection ops whilie initialising the pool
Modified:
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
Modified:
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
URL:
http://svn.apache.org/viewvc/mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java?rev=789164r1=789163r2=789164view=diff
==
---
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
(original)
+++
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
Sun Jun 28 22:30:02 2009
@@ -19,6 +19,7 @@
*/
package org.apache.mina.core.service;
+import java.lang.reflect.Constructor;
import java.util.concurrent.Executor;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@@ -73,44 +74,57 @@
* @param T the type of the {...@link IoSession} to be managed by the
specified
* �...@link IoProcessor}.
*/
-public class SimpleIoProcessorPoolT extends AbstractIoSession implements
IoProcessorT {
-
- private static final int DEFAULT_SIZE =
Runtime.getRuntime().availableProcessors() + 1;
- private static final AttributeKey PROCESSOR = new
AttributeKey(SimpleIoProcessorPool.class, processor);
-
- private final static Logger LOGGER =
LoggerFactory.getLogger(SimpleIoProcessorPool.class);
+public class SimpleIoProcessorPoolT extends AbstractIoSession implements
+ IoProcessorT {
+
+ private static final int DEFAULT_SIZE = Runtime.getRuntime()
+ .availableProcessors() + 1;
+
+ private static final AttributeKey PROCESSOR = new AttributeKey(
+ SimpleIoProcessorPool.class, processor);
+
+ private final static Logger LOGGER = LoggerFactory
+ .getLogger(SimpleIoProcessorPool.class);
private final IoProcessorT[] pool;
+
private final AtomicInteger processorDistributor = new AtomicInteger();
+
private final Executor executor;
+
private final boolean createdExecutor;
-
+
private final Object disposalLock = new Object();
+
private volatile boolean disposing;
+
private volatile boolean disposed;
-
+
public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType) {
this(processorType, null, DEFAULT_SIZE);
}
-
- public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType, int size) {
+
+ public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType,
+ int size) {
this(processorType, null, size);
}
- public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType, Executor executor) {
+ public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType,
+ Executor executor) {
this(processorType, executor, DEFAULT_SIZE);
}
-
+
@SuppressWarnings(unchecked)
- public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType, Executor executor, int size) {
+ public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType,
+ Executor executor, int size) {
if (processorType == null) {
throw new NullPointerException(processorType);
}
if (size = 0) {
- throw new IllegalArgumentException(
- size: + size + (expected: positive integer));
+ throw new IllegalArgumentException(size: + size
+ + (expected: positive integer));
}
-
+
if (executor == null) {
this.executor = executor = Executors.newCachedThreadPool();
this.createdExecutor = true;
@@ -118,56 +132,72 @@
this.executor = executor;
this.createdExecutor = false;
}
-
+
pool = new IoProcessor[size];
-
+
boolean success = false;
+ Constructor? extends IoProcessorT processorConstructor = null;
+ boolean usesExecutorArg = true;
+
try {
- for (int i = 0; i pool.length; i ++) {
- IoProcessorT processor = null;
-
- // Try to create a new processor with a proper constructor.
+ // We create at least one processor
+ try {
try {
- try {
- processor =
processorType.getConstructor(ExecutorService.class).newInstance(executor);
- } catch (NoSuchMethodException e) {
- // To the next step...
- }
[jira] Commented: (FTPSERVER-315) Pass FtpSession information to the UserManager.authenticate method
[ https://issues.apache.org/jira/browse/FTPSERVER-315?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12725145#action_12725145 ] Sai Pullabhotla commented on FTPSERVER-315: --- Another reason where this change would be useful is: If I want to force some users (not all) log in to the FTPS server using client certificate and passwords, I could use the FtpSession.getClientCertificates() method in the authenticate method to determine if the user should be allowed access or not. Pass FtpSession information to the UserManager.authenticate method -- Key: FTPSERVER-315 URL: https://issues.apache.org/jira/browse/FTPSERVER-315 Project: FtpServer Issue Type: Improvement Components: Core, Ftplets Reporter: Sai Pullabhotla Priority: Minor Fix For: 2.0.0 Currently the UserManager interface has the authenticate method defined as follows: User authenticate(Authentication authentication) throws AuthenticationFailedException; I'm wondering if it would be of any benefit to change it to: User authenticate(Authentication authentication, FtpSession session) throws AuthenticationFailedException; The reason(s) behind this - I want to log a message when the login fails. The login could fail to due to a number of reasons - such as Account is disabled, password has expired and so on. Since I do not have the session information available from this interface, I'm not able to log all the information that I normally do - such as the session ID, remote address and so on. I know I can log this information from onLogin() method of an Ftplet, but then I would not have any information on why the login has actually failed. All I've is - 530 Authentication Failed reply. Another benefit would be if I want to implement my user manager based on user name and IP address. For example let User1 login if and only if he is connecting from IP address xxx.xxx.xxx.xxx. Not sure if any one does this kind of authentication, but in case if some one want to, this change should help. More info about this feature request can be found in the thread - http://www.mail-archive.com/dev@mina.apache.org/msg12942.html. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (FTPSERVER-315) Pass FtpSession information to the UserManager.authenticate method
[ https://issues.apache.org/jira/browse/FTPSERVER-315?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12725150#action_12725150 ] Niklas Gustavsson commented on FTPSERVER-315: - The client certificate chain is already available to the UserManager in the UserMetadata class (calling authentication.getUserMetadata().getCertificateChain()). Pass FtpSession information to the UserManager.authenticate method -- Key: FTPSERVER-315 URL: https://issues.apache.org/jira/browse/FTPSERVER-315 Project: FtpServer Issue Type: Improvement Components: Core, Ftplets Reporter: Sai Pullabhotla Priority: Minor Fix For: 2.0.0 Currently the UserManager interface has the authenticate method defined as follows: User authenticate(Authentication authentication) throws AuthenticationFailedException; I'm wondering if it would be of any benefit to change it to: User authenticate(Authentication authentication, FtpSession session) throws AuthenticationFailedException; The reason(s) behind this - I want to log a message when the login fails. The login could fail to due to a number of reasons - such as Account is disabled, password has expired and so on. Since I do not have the session information available from this interface, I'm not able to log all the information that I normally do - such as the session ID, remote address and so on. I know I can log this information from onLogin() method of an Ftplet, but then I would not have any information on why the login has actually failed. All I've is - 530 Authentication Failed reply. Another benefit would be if I want to implement my user manager based on user name and IP address. For example let User1 login if and only if he is connecting from IP address xxx.xxx.xxx.xxx. Not sure if any one does this kind of authentication, but in case if some one want to, this change should help. More info about this feature request can be found in the thread - http://www.mail-archive.com/dev@mina.apache.org/msg12942.html. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
Re : svn commit: r789164 - /mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
Yes
in previous version each constructor type was tested again and again in the
loop
The new code is slightly more complex but it is based on the fact that we need
at least one processor
so we discover which constructor to use and then we loop and use the previously
determined constructor.
Cordialement, Regards,
-Edouard De Oliveira-
Blog: http://tedorgwp.free.fr
WebSite: http://tedorg.free.fr/en/main.php
- Message d'origine
De : Julien Vermillard jvermill...@apache.org
À : dev@mina.apache.org
Envoyé le : Lundi, 29 Juin 2009, 11h13mn 24s
Objet : Re: svn commit: r789164 -
/mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
Hi,
Interesting optimization. The idea is to make SimpleIoPPool
instantiation faster ?
Julien
On Mon, Jun 29, 2009 at 12:30 AM, edeolive...@apache.org wrote:
Author: edeoliveira
Date: Sun Jun 28 22:30:02 2009
New Revision: 789164
URL: http://svn.apache.org/viewvc?rev=789164view=rev
Log:
Fix to do less reflection ops whilie initialising the pool
Modified:
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
Modified:
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
URL:
http://svn.apache.org/viewvc/mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java?rev=789164r1=789163r2=789164view=diff
==
---
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
(original)
+++
mina/trunk/core/src/main/java/org/apache/mina/core/service/SimpleIoProcessorPool.java
Sun Jun 28 22:30:02 2009
@@ -19,6 +19,7 @@
*/
package org.apache.mina.core.service;
+import java.lang.reflect.Constructor;
import java.util.concurrent.Executor;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@@ -73,44 +74,57 @@
* @param T the type of the {...@link IoSession} to be managed by the
specified
*{...@link IoProcessor}.
*/
-public class SimpleIoProcessorPoolT extends AbstractIoSession implements
IoProcessorT {
-
-private static final int DEFAULT_SIZE =
Runtime.getRuntime().availableProcessors() + 1;
-private static final AttributeKey PROCESSOR = new
AttributeKey(SimpleIoProcessorPool.class, processor);
-
-private final static Logger LOGGER =
LoggerFactory.getLogger(SimpleIoProcessorPool.class);
+public class SimpleIoProcessorPoolT extends AbstractIoSession implements
+IoProcessorT {
+
+private static final int DEFAULT_SIZE = Runtime.getRuntime()
+.availableProcessors() + 1;
+
+private static final AttributeKey PROCESSOR = new AttributeKey(
+SimpleIoProcessorPool.class, processor);
+
+private final static Logger LOGGER = LoggerFactory
+.getLogger(SimpleIoProcessorPool.class);
private final IoProcessorT[] pool;
+
private final AtomicInteger processorDistributor = new AtomicInteger();
+
private final Executor executor;
+
private final boolean createdExecutor;
-
+
private final Object disposalLock = new Object();
+
private volatile boolean disposing;
+
private volatile boolean disposed;
-
+
public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType) {
this(processorType, null, DEFAULT_SIZE);
}
-
-public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType, int size) {
+
+public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType,
+int size) {
this(processorType, null, size);
}
-public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType, Executor executor) {
+public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType,
+Executor executor) {
this(processorType, executor, DEFAULT_SIZE);
}
-
+
@SuppressWarnings(unchecked)
-public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType, Executor executor, int size) {
+public SimpleIoProcessorPool(Class? extends IoProcessorT
processorType,
+Executor executor, int size) {
if (processorType == null) {
throw new NullPointerException(processorType);
}
if (size = 0) {
-throw new IllegalArgumentException(
-size: + size + (expected: positive integer));
+throw new IllegalArgumentException(size: + size
++ (expected: positive integer));
}
-
+
if (executor == null) {
this.executor = executor = Executors.newCachedThreadPool();
this.createdExecutor = true;
@@ -118,56 +132,72 @@
this.executor = executor;
this.createdExecutor = false;
}
-
+
pool = new IoProcessor[size];
-
+
boolean success =
Re : About MINA commits with some code refactoring
+1 You're perfectly right to prefer spam instead of difficult commits to read ^^ Cordialement, Regards, -Edouard De Oliveira- Blog: http://tedorgwp.free.fr WebSite: http://tedorg.free.fr/en/main.php - Message d'origine De : Julien Vermillard jvermill...@archean.fr À : dev@mina.apache.org Envoyé le : Lundi, 29 Juin 2009, 11h11mn 20s Objet : Re: About MINA commits with some code refactoring Le Mon, 29 Jun 2009 11:08:33 +0200, Emmanuel Lecharny elecha...@apache.org a écrit : Hi guys, recently I saw some commits on MINA where some code has been refactored (like lines were limited to 80 caracters, stuff like that). I don't think it's a bad thing to do code refactoring, but the problem is that when it's mixed with some code addition, it buries the fixes into a lot of refactoring, and it's difficult for reviewers to check the fixes. My personal guess is that those who are committing such code are using some specific options on their favorite IDE, leading to such injection of code. It's not only about line splitting, it can be addition of windows style EOL, or even tabs. I'm not free from such injections, I must admit. There is a Eclipse formating file for those who are using Eclipse, that must be used in order to avoid such automatic refactoring : http://mina.apache.org/developer-guide.html#DeveloperGuide-CodingConvention If you are using another IDE, it could be cool to create the same kind of file. Last, not least, if you think the file need to be refactored, feel free to do so, but *please*, do that in a separate commit : first refactor the code, commit it and then fix the code and commit again. Many thanks ! +1 It's painful to decipher some commits. Julien
Re: Re : About MINA commits with some code refactoring
Edouard De Oliveira wrote: +1 You're perfectly right to prefer spam instead of difficult commits to read ^^ Just for my information, which IDE are you using ? -- -- cordialement, regards, Emmanuel Lécharny www.iktek.com directory.apache.org
Re : Re : About MINA commits with some code refactoring
Eclipse and i do use the convention formatting file usually i work under windows but in the past two weeks i've been using it in sun's virtualbox running latest ubuntu Cordialement, Regards, -Edouard De Oliveira- Blog: http://tedorgwp.free.fr WebSite: http://tedorg.free.fr/en/main.php - Message d'origine De : Emmanuel Lecharny elecha...@apache.org À : dev@mina.apache.org Envoyé le : Lundi, 29 Juin 2009, 15h08mn 18s Objet : Re: Re : About MINA commits with some code refactoring Edouard De Oliveira wrote: +1 You're perfectly right to prefer spam instead of difficult commits to read ^^ Just for my information, which IDE are you using ? -- -- cordialement, regards, Emmanuel Lécharny www.iktek.com directory.apache.org
Re: [jira] Commented: (FTPSERVER-315) Pass FtpSession information to the UserManager.authenticate method
Thanks, Niklas. I thought there was something like that, but overlooked. Sai Pullabhotla www.jMethods.com On Mon, Jun 29, 2009 at 7:25 AM, Niklas Gustavsson (JIRA) j...@apache.orgwrote: [ https://issues.apache.org/jira/browse/FTPSERVER-315?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12725150#action_12725150] Niklas Gustavsson commented on FTPSERVER-315: - The client certificate chain is already available to the UserManager in the UserMetadata class (calling authentication.getUserMetadata().getCertificateChain()). Pass FtpSession information to the UserManager.authenticate method -- Key: FTPSERVER-315 URL: https://issues.apache.org/jira/browse/FTPSERVER-315 Project: FtpServer Issue Type: Improvement Components: Core, Ftplets Reporter: Sai Pullabhotla Priority: Minor Fix For: 2.0.0 Currently the UserManager interface has the authenticate method defined as follows: User authenticate(Authentication authentication) throws AuthenticationFailedException; I'm wondering if it would be of any benefit to change it to: User authenticate(Authentication authentication, FtpSession session) throws AuthenticationFailedException; The reason(s) behind this - I want to log a message when the login fails. The login could fail to due to a number of reasons - such as Account is disabled, password has expired and so on. Since I do not have the session information available from this interface, I'm not able to log all the information that I normally do - such as the session ID, remote address and so on. I know I can log this information from onLogin() method of an Ftplet, but then I would not have any information on why the login has actually failed. All I've is - 530 Authentication Failed reply. Another benefit would be if I want to implement my user manager based on user name and IP address. For example let User1 login if and only if he is connecting from IP address xxx.xxx.xxx.xxx. Not sure if any one does this kind of authentication, but in case if some one want to, this change should help. More info about this feature request can be found in the thread - http://www.mail-archive.com/dev@mina.apache.org/msg12942.html. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
