Accumulo
  1. Accumulo
  2. ACCUMULO-2325

Avoid repeated checking in addViolation in NumericValueConstraint and AlphaNumKeyContraint

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.7.0
    • Component/s: None
    • Labels:
      None

      Description

      addViolation is very verbose and does check for null on every violation. Instead we can initialize violations as non-null just return null from check if the list is empty.

        Activity

        Vikram Srivastava created issue -
        Hide
        Vikram Srivastava added a comment -

        Attached patch against master.

        Show
        Vikram Srivastava added a comment - Attached patch against master.
        Vikram Srivastava made changes -
        Field Original Value New Value
        Attachment ACCUMULO-2325.v1.patch.txt [ 12627099 ]
        Vikram Srivastava made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Fix Version/s 1.7.0 [ 12324607 ]
        Josh Elser made changes -
        Issue Type Bug [ 1 ] Improvement [ 4 ]
        Christopher Tubbs made changes -
        Affects Version/s 1.7.0 [ 12324607 ]
        Hide
        Josh Elser added a comment -

        I'll try to take a look at this. Sorry for it sitting for so long.

        Show
        Josh Elser added a comment - I'll try to take a look at this. Sorry for it sitting for so long.
        Josh Elser made changes -
        Priority Minor [ 4 ] Blocker [ 1 ]
        ASF subversion and git services logged work - 07/Apr/15 15:45
        • Time Spent:
          10m
           
          Commit edc080c830d2e9689e973a8b8b3447d3bbf661b5 in accumulo's branch refs/heads/master from [~elserj]
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=edc080c ]

          ACCUMULO-2325 Attempt to make example constraints more efficient.

          Partially applied patch from Vikram Srivastava. Due to the nature
          of constraints, it is far more likely that it will never fire. As
          such, it makes sense that we optimize for that case. Avoiding the
          allocation of a new collection (as the code already did) is far
          better than always allocating a new list/set for every invocation
          of the constraint. Tests were directly applied from the original patch.
        ASF subversion and git services made changes -
        Remaining Estimate 0h [ 0 ]
        Time Spent 10m [ 600 ]
        Worklog Id 21089 [ 21089 ]
        Hide
        Josh Elser added a comment -

        Again, patch did not apply cleanly. I did not make all of the changes contained in the patch because a constraint should avoid allocations of a new List every time it is invoked. In the normal case, a constraint should not fire. As such, it is already an optimization to avoid this allocation and perform a quick null check.

        The tests that Vikram provided were great and those were applied, as was the modification to use a Set (to make a quick contains call) and make a list at the end of the method.

        Show
        Josh Elser added a comment - Again, patch did not apply cleanly. I did not make all of the changes contained in the patch because a constraint should avoid allocations of a new List every time it is invoked. In the normal case, a constraint should not fire. As such, it is already an optimization to avoid this allocation and perform a quick null check. The tests that Vikram provided were great and those were applied, as was the modification to use a Set (to make a quick contains call) and make a list at the end of the method.
        Josh Elser made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        4h 19m 1 Vikram Srivastava 05/Feb/14 09:15
        Patch Available Patch Available Resolved Resolved
        426d 5h 39m 1 Josh Elser 07/Apr/15 15:54

          People

          • Assignee:
            Vikram Srivastava
            Reporter:
            Vikram Srivastava
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - Not Specified
              Not Specified
              Remaining:
              Remaining Estimate - 0h
              0h
              Logged:
              Time Spent - 10m
              10m

                Development