Skip to content

Conversation

@gfigiel
Copy link
Collaborator

@gfigiel gfigiel commented Jan 13, 2017

Loadbalancing and failover extension implementation + checkstyle corrections for issues indicated during build & master branch merge.

Copy link
Contributor

@brainslog brainslog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gfigiel, thanks for this great contribution. I would ask you if you could break this into smaller, more scoped PRs, as it is hard to review with too many changes and some unrelated to the PR topic (eg: removing "~" files, fixing documentation issues, etc) and try to avoid unnecessary changes (indentation, import changes, else/catch placement, etc) as they hide the actual changes, please do those that apply at a separate PR and they can be quickly merged (thanks for spotting them!)

Also, would be good to provide some more context on what changed, how to use it, a summary of the modifications and some explanation to it to make the review easier.

With regards to license headers, for new files, please use the Telestax license header (as in

/*
* TeleStax, Open Source Cloud Communications
* Copyright 2011-2016, TeleStax Inc. and individual contributors
* by the @authors tag.
*
* This program is free software: you can redistribute it and/or modify
* under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation; either version 3 of
* the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>
*/
) and for modified files with JBoss header, the dual license header (as in
/*
* TeleStax, Open Source Cloud Communications
* Copyright 2011-2016, TeleStax Inc. and individual contributors
* by the @authors tag.
*
* This program is free software: you can redistribute it and/or modify
* under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation; either version 3 of
* the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
* This file incorporates work covered by the following copyright and
* permission notice:
*
* JBoss, Home of Professional Open Source
* Copyright 2007-2011, Red Hat, Inc. and individual contributors
* by the @authors tag. See the copyright.txt in the distribution for a
* full listing of individual contributors.
*
* This is free software; you can redistribute it and/or modify it
* under the terms of the GNU Lesser General Public License as
* published by the Free Software Foundation; either version 2.1 of
* the License, or (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with this software; if not, write to the Free
* Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
* 02110-1301 USA, or see the FSF site: http://www.fsf.org.
*/
)

Some general comments from a quick review (sorry if I misunderstood anything, on the scoped PRs should be easier to keep up) but there seems to be some things which are application specific (like the added org.jdiameter.api.RequestType which is CCA/Ro specific but added at general API, the TX_TIMEOUT parameter which is also CCA/Ro specific but added to general configuration) and some other stuff that should be more generic and it's implemented as part of the App Session (shouldn't the retransmission timer be made at a higher level and handled by the stack core instead of App session?)

I will still take some more time to grasp all the changes, but just letting you know these in advance so if you could go with breaking and cleaning the PR, that would be great and allow for more detailed discussion.

Thanks! Awesome contribution!

@brainslog brainslog modified the milestones: 1.7.0.FINAL, 1.8.0.FINAL Mar 28, 2017
@bkrok
Copy link

bkrok commented Apr 7, 2017

Commit c92964a contains:

  • fixes for failover enchancement
  • handling CC-Session-Failover AVP
  • Request Timeout event enchancement (before Request Tx Timeout was named Request Timeout)

@ammendonca
Copy link
Contributor

Hi @gfigiel, @bkrok,

Can you please reduce the scope of this PR to the changes regarding the functionality we are trying to implement with it ? There's too much noise with 3,362 additions and 32,441 deletions for proper review and merge.

I'll be happy to review and merge the other fixes you have made along, but in separate PRs.

Also, if you could try to explain better what you are trying to achieve and address some of the technical questions I've posted, would be great.

Thanks!

@ammendonca ammendonca removed this from the 1.7.0-FINAL-Sprint 3 milestone Jun 29, 2017
@ammendonca
Copy link
Contributor

Closing as there's a new Pull Request on #97

@ammendonca ammendonca closed this Jun 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants