You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fortress@directory.apache.org by Yudhi Karunia Surtan <yu...@apache.org> on 2018/02/12 18:32:48 UTC
Process of making changes at trunk
Greetings,
I'm planning to make a change for fortress rest util connection pooling
mechanism.
Currently we only used 2 connections for the connection pooling as the
default value provided by apache http client library.
Apart from the connection pool, i also look that it would be good if we
could manage the heap memory management in string using StringUtils.join
method.
Since that library used and simplify the StringBuilder mechanism and the
most benefit is to maintain the GC time become very minimum. I know from
the performance point of view StringBuilder will make a bit slow compare to
the concatenation one.
However, if we considering the greater good of GC, StringBuilder can reduce
the old GC period since the objects will be discarded before it reach old
GC because it will not wait until the strong string reference got destroyed.
As per my github pull request shawn asked me to put that concern at the
mailing list, unfortunatelly i can't find the exact svn at
https://svn.apache.org/viewvc/directory/trunks/ for fortress project.
Please guide how to commit my code contribution.
Re: Process of making changes at trunk
Posted by Shawn McKinney <sm...@apache.org>.
> On Apr 4, 2018, at 11:47 PM, Emmanuel Lécharny <el...@gmail.com> wrote:
>
> Le 05/04/2018 à 00:57, Shawn McKinney a écrit :
>>
>>> On Apr 4, 2018, at 4:18 AM, Emmanuel Lécharny <el...@gmail.com> wrote:
>>>
>>> One more thing :
>>>
>>> if ( props.size() > 0 )
>>>
>>> should be
>>>
>>> if ( !props.isEmpty() )
>>
>> That won’t work, props is not java.util.Properties, it’s:
>
> List<Props.Entry> props = inProps.getEntry();
>
> So it *must* implement List.isEmpty()...
I stand corrected. Thanks
Re: Process of making changes at trunk
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 05/04/2018 à 00:57, Shawn McKinney a écrit :
>
>> On Apr 4, 2018, at 4:18 AM, Emmanuel Lécharny <el...@gmail.com> wrote:
>>
>> One more thing :
>>
>> if ( props.size() > 0 )
>>
>> should be
>>
>> if ( !props.isEmpty() )
>
> That won’t work, props is not java.util.Properties, it’s:
List<Props.Entry> props = inProps.getEntry();
So it *must* implement List.isEmpty()...
--
Emmanuel Lecharny
Symas.com
directory.apache.org
Re: Process of making changes at trunk
Posted by Shawn McKinney <sm...@apache.org>.
> On Apr 4, 2018, at 4:18 AM, Emmanuel Lécharny <el...@gmail.com> wrote:
>
> One more thing :
>
> if ( props.size() > 0 )
>
> should be
>
> if ( !props.isEmpty() )
That won’t work, props is not java.util.Properties, it’s:
public class Props extends FortEntity implements Serializable
{
/** Default serialVersionUID */
private static final long serialVersionUID = 1L;
private List<Props.Entry> entry;
Sort of a container class to carry properties as a list of name/values when it’s time to serialize via jaxb. The rationale for converting escapes me atm, other than it (serialization) didn’t work with attribute type of java.util.Properties.
Might be worth a revisit, to clean this up, i.e. use the java.util version and not this other one. We’d be able to also remove the redundant code Yudhi mentioned, i.e. using the delete key.
Shawn
Re: Process of making changes at trunk
Posted by Emmanuel Lécharny <el...@gmail.com>.
Le 04/04/2018 à 09:26, Yudhi Karunia Surtan a écrit :
> Guys,
>
> Finally make my first commit to apache git repository.
> Thanks for guide me until here.
>
>
> I also find there are some duplication for some code like this :
>
> Properties properties = null;
> List<Props.Entry> props = inProps.getEntry();
> if ( props.size() > 0 )
> {
> properties = new Properties();
> //int size = props.size();
> for ( Props.Entry entry : props )
> {
> String key = entry.getKey();
> String val = entry.getValue();
> properties.setProperty( key, val );
> }
> }
> return properties;
>
>
> at org.apache.directory.fortress.core.model.Group,
> org.apache.directory.fortress.core.model.Permission,
> org.apache.directory.fortress.core.model.PermObj,
> org.apache.directory.fortress.core.model.Role,
> org.apache.directory.fortress.core.model.User.
>
> Currently we have RestUtils.getProperties What do you think if above
> classes using RestUtils method to remove duplication? However, if we do
> that from the dependency point of view, i think it is a bit awkward because
> "model" package depends on "rest" package.
I do think an utility function should be added to fortress-core, as each
project depends on it. It makes no sense to move that to the
fortress-rest module, IMHO.
One more thing :
if ( props.size() > 0 )
should be
if ( !props.isEmpty() )
And, yes, +1 to remove code duplication :-)
--
Emmanuel Lecharny
Symas.com
directory.apache.org
Re: Process of making changes at trunk
Posted by Shawn McKinney <sm...@apache.org>.
> On Apr 4, 2018, at 2:26 AM, Yudhi Karunia Surtan <yu...@apache.org> wrote:
>
> Currently we have RestUtils.getProperties What do you think if above
> classes using RestUtils method to remove duplication? However, if we do
> that from the dependency point of view, i think it is a bit awkward because
> "model" package depends on "rest" package.
Agreed, how about we create a sep utils in the model package and move the code into there?
Re: Process of making changes at trunk
Posted by Yudhi Karunia Surtan <yu...@apache.org>.
Guys,
Finally make my first commit to apache git repository.
Thanks for guide me until here.
I also find there are some duplication for some code like this :
Properties properties = null;
List<Props.Entry> props = inProps.getEntry();
if ( props.size() > 0 )
{
properties = new Properties();
//int size = props.size();
for ( Props.Entry entry : props )
{
String key = entry.getKey();
String val = entry.getValue();
properties.setProperty( key, val );
}
}
return properties;
at org.apache.directory.fortress.core.model.Group,
org.apache.directory.fortress.core.model.Permission,
org.apache.directory.fortress.core.model.PermObj,
org.apache.directory.fortress.core.model.Role,
org.apache.directory.fortress.core.model.User.
Currently we have RestUtils.getProperties What do you think if above
classes using RestUtils method to remove duplication? However, if we do
that from the dependency point of view, i think it is a bit awkward because
"model" package depends on "rest" package.
On Tue, Feb 13, 2018 at 10:09 PM, Shawn McKinney <sm...@apache.org>
wrote:
>
> > On Feb 13, 2018, at 9:06 AM, Shawn McKinney <sm...@apache.org>
> wrote:
> >
> > I always create an issue in JIRA, with a detailed description of the
> problem and the fix, and use the jira issue #, along with a short desc, in
> the commit comments.
> >
> > e.g. FC-231 - DAO's should be package private
> >
> > That way we can tie the change back to the issue, where longer
> description of the change may take place.
>
> Another advantage of using JIRA to manage issues, we have a nice list of
> changes, for the release notes.
Re: Process of making changes at trunk
Posted by Shawn McKinney <sm...@apache.org>.
> On Feb 13, 2018, at 9:06 AM, Shawn McKinney <sm...@apache.org> wrote:
>
> I always create an issue in JIRA, with a detailed description of the problem and the fix, and use the jira issue #, along with a short desc, in the commit comments.
>
> e.g. FC-231 - DAO's should be package private
>
> That way we can tie the change back to the issue, where longer description of the change may take place.
Another advantage of using JIRA to manage issues, we have a nice list of changes, for the release notes.
Re: Process of making changes at trunk
Posted by Shawn McKinney <sm...@apache.org>.
> On Feb 12, 2018, at 9:14 PM, Yudhi Karunia Surtan <br...@gmail.com> wrote:
>
> Do I need to create an issue and create a new branch, or just commit the
> changes to the master branch?
Hi Yudhi,
Typically when we make a change, we’ll notify the project about the issue, the fix, etc, (which you have already done). I always create an issue in JIRA, with a detailed description of the problem and the fix, and use the jira issue #, along with a short desc, in the commit comments.
e.g. FC-231 - DAO's should be package private
That way we can tie the change back to the issue, where longer description of the change may take place.
For something trivial, like a code comment, or some other self-explanatory change, I will skip the step to create the JIRA issue, and just commit, with the short comment.
For really disruptive changes, i.e. changing the API, data structures, we discuss on this ML the pros/cons, reach consensus, before going forward. In these cases we might create a branch, because of the duration it takes to complete the work.
For simple changes, like that which you’ve brought up now, simply committing directly to trunk is fine.
One word of caution, I always make sure the junit tests pass successfully before checking in a change. The idea if the tests pass, no regressions, or other known bugs have been inserted. If the tests pass, and a bug is introduce anyway, we know its time to add a new test case.
Let me know if you have any questions about how to do this, which tests, etc.
Good questions btw, please feel welcomed to continue asking these questions here. One of these days I’m going to document these procedures, for our coding handbook, in the meantime, apologize for the tribal knowledge. :-)
Shawn
Re: Process of making changes at trunk
Posted by Yudhi Karunia Surtan <br...@gmail.com>.
Hi Stefan,
Ah, I see..
Thanks for your answer.
Do I need to create an issue and create a new branch, or just commit the
changes to the master branch?
On Feb 13, 2018 1:55 AM, "Stefan Seelmann" <ma...@stefan-seelmann.de> wrote:
> Hi Yudhi,
>
> On 02/12/2018 07:32 PM, Yudhi Karunia Surtan wrote:
> > Greetings,
> >
> > I'm planning to make a change for fortress rest util connection pooling
> > mechanism.
> > Currently we only used 2 connections for the connection pooling as the
> > default value provided by apache http client library.
> > Apart from the connection pool, i also look that it would be good if we
> > could manage the heap memory management in string using StringUtils.join
> > method.
> >
> > Since that library used and simplify the StringBuilder mechanism and the
> > most benefit is to maintain the GC time become very minimum. I know from
> > the performance point of view StringBuilder will make a bit slow compare
> to
> > the concatenation one.
> > However, if we considering the greater good of GC, StringBuilder can
> reduce
> > the old GC period since the objects will be discarded before it reach old
> > GC because it will not wait until the strong string reference got
> destroyed.
> >
> > As per my github pull request shawn asked me to put that concern at the
> > mailing list, unfortunatelly i can't find the exact svn at
> > https://svn.apache.org/viewvc/directory/trunks/ for fortress project.
> > Please guide how to commit my code contribution.
>
> Fortress already uses git, not SVN, you can find the repository URLs
> here: https://directory.apache.org/sources.html You can just git clone
> those and then push via HTTPS using your Apache user and password.
>
> Fortress still uses the old git-wip-us service, it would make sense to
> move it to gitbox which would then allow bidirectional sync with GitHub.
>
> Kind Regards,
> Stefan
>
Re: Process of making changes at trunk
Posted by Stefan Seelmann <ma...@stefan-seelmann.de>.
Hi Yudhi,
On 02/12/2018 07:32 PM, Yudhi Karunia Surtan wrote:
> Greetings,
>
> I'm planning to make a change for fortress rest util connection pooling
> mechanism.
> Currently we only used 2 connections for the connection pooling as the
> default value provided by apache http client library.
> Apart from the connection pool, i also look that it would be good if we
> could manage the heap memory management in string using StringUtils.join
> method.
>
> Since that library used and simplify the StringBuilder mechanism and the
> most benefit is to maintain the GC time become very minimum. I know from
> the performance point of view StringBuilder will make a bit slow compare to
> the concatenation one.
> However, if we considering the greater good of GC, StringBuilder can reduce
> the old GC period since the objects will be discarded before it reach old
> GC because it will not wait until the strong string reference got destroyed.
>
> As per my github pull request shawn asked me to put that concern at the
> mailing list, unfortunatelly i can't find the exact svn at
> https://svn.apache.org/viewvc/directory/trunks/ for fortress project.
> Please guide how to commit my code contribution.
Fortress already uses git, not SVN, you can find the repository URLs
here: https://directory.apache.org/sources.html You can just git clone
those and then push via HTTPS using your Apache user and password.
Fortress still uses the old git-wip-us service, it would make sense to
move it to gitbox which would then allow bidirectional sync with GitHub.
Kind Regards,
Stefan