New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCCP rules sorting based on ID #249

Closed
faizann opened this Issue Jul 12, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@faizann
Contributor

faizann commented Jul 12, 2017

Currently SCCP rules are sorted based on calledDigits and callingDigits patterns. This gets confusing to know which rule is checked first while rule matching. The GUI displays rules sorted based on ID.
It is better to sort the rules internally for matching based on RuleID as well. This will make it cleaner.
Also there is a bug in callingDigits rule sorting in case where there are 2 rules with same calledDigits but 1 of them has callingDigits and there are 1 or more rules with only calledDigits.
The comparison between calledDigits rules can in some cases put the callingDigits rule below in the order and hence doesn't get matched. A fix is this but still it is better that we just simplify and use RuleIDs for sorting.


// if rule1 has a calling party and rule2 does not then put rule 1 above and do check vice versa
        if (o1.getPatternCallingAddress()!=null && o2.getPatternCallingAddress() == null)
            return -1;
        if ( o1.getPatternCallingAddress() == null && o2.getPatternCallingAddress() != null )
            return 1;

@vetss

This comment has been minimized.

Collaborator

vetss commented Jul 20, 2017

Hello @faizann

  1. I see your point for manually sorting of rules (depending on ID or something like it). Now we have sorting depending on digits (liek more automated). We need some significant reason to change the behavior.

  2. This is a current code. I have not found significant changing as compared with your version. Can you please comment.

        // Check if digits are exactly same. In that case we sort based on the callingDigits
        if ( digits1.equals( digits2 )) {
            // if rule1 has calling party and rule2 doesn't then we put rule1 first
            if ( o1.getPatternCallingAddress() != null && o2.getPatternCallingAddress() == null ) {
                return -1;
            } else if ( o1.getPatternCallingAddress() == null && o2.getPatternCallingAddress() != null ) {
                return 1;
            } else if ( o1.getPatternCallingAddress() != null && o2.getPatternCallingAddress() != null ) {
                // both have calling party addresses. lets compare these 2
                digits1 = o1.getPatternCallingAddress().getGlobalTitle().getDigits();
                digits2 = o2.getPatternCallingAddress().getGlobalTitle().getDigits();

                // Normalize rule. Remove all separator
                digits1 = digits1.replaceAll(SECTION_SEPARTOR, "");
                digits2 = digits2.replaceAll(SECTION_SEPARTOR, "");

                return compareDigits( digits1, digits2 );
            }
        }
        return compareDigits( digits1, digits2 );
@faizann

This comment has been minimized.

Contributor

faizann commented Jul 20, 2017

Hi @vetss

The change is significant in functionality but is not a lot of code lines. I didn't delete the code but just commented out the older rules. You can see at this line I am only comparing IDs and returning that as the result of comparator. I also commented out all the tests.
8a40032#diff-bdd35461f53c8db4e2c28c93103d8f16R72

@faizann

This comment has been minimized.

Contributor

faizann commented Jul 20, 2017

My reasoning for change is that there is already a bug in my previous PR where we sort based on callingParty if calledParty addresses are same. That was a flawed logic as that still doesn't give proper sorting when comparing a rule which has callingParty but a different calledParty. Hence that randomness is highly dangerous and I realised that it gets complicated to keep track of sorting. ID based rules sorting gives a very clear view of what gets matched first and then has a natural fall through process.

@vetss

This comment has been minimized.

Collaborator

vetss commented Jul 20, 2017

Hello @faizann

The change is significant in functionality but is not a lot of code lines. I didn't delete the code but just commented out the older rules. You can see at this line I am only comparing IDs and returning that as the result of comparator. I also commented out all the tests.

I was asking for the case "Also there is a bug in callingDigits rule sorting in case where there are 2 rules with same calledDigits but 1 of them has callingDigits and there are 1 or more rules with only calledDigits.". I do not possibly still understand what is a bug in current implementation. If you see it please clarify more, better in soce update suggestion.

that randomness is highly dangerous and I realised that it gets complicated to keep track of sorting.

Please describe more what you see dangerous (may be examples). I am asking because just changing of behavior will affect current customers and there must be a big reason to do it. We need to care in this case of backward compatibility when old configs will be auto converted into a new style (possibly with proper ID changing) + manual update announsing.

@faizann

This comment has been minimized.

Contributor

faizann commented Jul 25, 2017

Hi @vetss

The problem with current sorting algorithm with both calling/called party is this.
Rule 1 calledParty 1234
Rule 2 calledParty 5678
Rule 3 calledParty 5678 callingParty 8888

The sorting goes like this
-- Rule 3 is put above Rule 2 because Rule 3 has callingParty
-- Rule 1 is put above Rule 3 as they both have same digit length and digits are different.
-- Rule 2 is put above Rule 1 as they both have same digit length and digits are different.
So, the order endsup being
Rule 2, Rule 1, Rule 3 which will be incorrect as Rule 3 is never matched. This happened to me and I realised that these sorting rules are too complicated to remember while creating rules. A simple ID based rule ordering is more logical to understand.

Of course we cannot just move to RuleID based sorting. I think we can allow the type of ordering based on a specific flag in SCCP general settings where users can pick legacy or new RuleID based ordering.

I can submit that code if you agree. For me RuleID based sorting was very helpful and solved a lot of headaches.

@vetss

This comment has been minimized.

Collaborator

vetss commented Jul 25, 2017

Hello @faizann

thanks for a detailed problem description.

  1. for a problem of sorting - can we add sorting depending on digits in compareDigits() ?

like replace:

    private int compareDigits( String digits1, String digits2 ) {
        // ............................
        return 1;
    }

with

    private int compareDigits( String digits1, String digits2 ) {
        // ............................
        return digits1.compareTo(digits2);
    }

?

  1. Befor switching to "sorting based on ID" lets think of all options / problems. I see your point for it, but as I described switching has also disadvantages.

@vetss vetss added this to the Sergey-8.0-sprint1 milestone Jul 25, 2017

@vetss

This comment has been minimized.

Collaborator

vetss commented Jul 31, 2017

Hello @faizann

after internal discussion I think that it is better "to introduce a Factory to load Rule algorithms, by default it would be shipped with the current RuleAlgorithm and config but you could add a new RuleAlgorithm to be able to make a better version of rule ordering and matching thus keeping backward compatibility but allowing new customers to move to the new rule ordering, once it is stable we can declare the previous one deprecated and remove it 1 or 2 major versions later."

So we agree for a possibility of both algos implementation with old algo by default. @faizann please let me know if it is good for you. Can you please also comment my comment for brinning to a liveness of the old algo ("1)" in my previous message).

@faizann

This comment has been minimized.

Contributor

faizann commented Jul 31, 2017

Hi @vetss

This is a very good idea indeed. I will work on a PR to get rule sorting based on your comments and test it.
I will also submit later the rule algorithms being loaded by a factory.

Thanks

faizann pushed a commit to faizann/jss7 that referenced this issue Aug 28, 2017

Faizan Naqvi
RestComm#249. Fixed SCCP rule sorting bug for callingParty as in some…
… cases rule sorting was wrong. Added RuleComparatorById to allow sorting of rules by ID. RuleComparator can be set using jboss-beans between legacy rulecomparator or rulecomparatorById

faizann pushed a commit to faizann/jss7 that referenced this issue Aug 28, 2017

faizann pushed a commit to faizann/jss7 that referenced this issue Aug 31, 2017

Faizan Naqvi
Fixed RestComm#249. 2 different sorting algorithms available. Legacy …
…rule sorting based on digits+wildcards is there. A bug where callingParty based rulesorting was giving wrong sorting order is fixed. New rulesorting algorithm based on RULE IDs is available and can be configured to use via jboss-beans.xml

@knosach knosach self-assigned this Dec 20, 2017

@knosach knosach closed this in 087c380 Dec 22, 2017

vetss added a commit to vetss/jss7 that referenced this issue Jan 2, 2018

knosach pushed a commit that referenced this issue Jan 26, 2018

knosach pushed a commit that referenced this issue Jan 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment