Cayenne
  1. Cayenne
  2. CAY-1573

QueryLogger to DI JdbcEventLogger migration

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.1M3
    • Component/s: None
    • Labels:
      None

      Description

      Migration from deprecated QueryLogger to DI enabled JdbcEventLogger.

      1. query-logger.patch
        168 kB
        Dzmitry Kazimirchyk
      2. query-logger.patch
        168 kB
        Dzmitry Kazimirchyk
      3. query-logger1.patch
        66 kB
        Dzmitry Kazimirchyk
      4. ql-adapters.patch
        86 kB
        Dzmitry Kazimirchyk
      5. ql-rest.patch
        33 kB
        Dzmitry Kazimirchyk

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Closed Closed
        40d 10h 5m 1 Andrus Adamchik 09/Jul/11 21:27
        Andrus Adamchik made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 3.1M3 [ 12316228 ]
        Resolution Fixed [ 1 ]
        Hide
        Andrus Adamchik added a comment -

        Both fresh patches applied with some refactoring on my part. Also figured we shouldn't keep QueryLogger around and removed it from Cayenne (this is pretty arbitrary on my part.... maybe we want to keep it for another cycle?)

        Show
        Andrus Adamchik added a comment - Both fresh patches applied with some refactoring on my part. Also figured we shouldn't keep QueryLogger around and removed it from Cayenne (this is pretty arbitrary on my part.... maybe we want to keep it for another cycle?)
        Dzmitry Kazimirchyk made changes -
        Attachment ql-adapters.patch [ 12485414 ]
        Attachment ql-rest.patch [ 12485415 ]
        Hide
        Dzmitry Kazimirchyk added a comment -

        Sorry for this, I can't apply it now too.
        Attaching separate patches for adapters and for the rest chunk.

        Show
        Dzmitry Kazimirchyk added a comment - Sorry for this, I can't apply it now too. Attaching separate patches for adapters and for the rest chunk.
        Hide
        Andrus Adamchik added a comment -

        Trying the patch from June 1. It doesn't apply at all. Checking the first class in the patch (framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/access/jdbc/BaseSQLAction.java) - it didn't have changes since March, so this is not a merge conflict. Still the lines are off... I wonder how it was generated? Probably the wrong baseline was used.

        Show
        Andrus Adamchik added a comment - Trying the patch from June 1. It doesn't apply at all. Checking the first class in the patch (framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/access/jdbc/BaseSQLAction.java) - it didn't have changes since March, so this is not a merge conflict. Still the lines are off... I wonder how it was generated? Probably the wrong baseline was used.
        Dzmitry Kazimirchyk made changes -
        Attachment query-logger1.patch [ 12481092 ]
        Hide
        Dzmitry Kazimirchyk added a comment -

        Attached patch with corrections for JdbcAdapter..BaseSQLAction chunk. Now initialization with NoopJdbcEventsLogger performed in constructor.
        Also, now all classes with direct reference to JdbcAdapter get logger directly from it without many levels of getters and setters.

        Show
        Dzmitry Kazimirchyk added a comment - Attached patch with corrections for JdbcAdapter..BaseSQLAction chunk. Now initialization with NoopJdbcEventsLogger performed in constructor. Also, now all classes with direct reference to JdbcAdapter get logger directly from it without many levels of getters and setters.
        Hide
        Andrus Adamchik added a comment -

        Thanks for the patch. Having it all in one piece is a bit hard to process, so I tried to separate into manageable pieces. I succeeded somewhat to split somewhat unrelated 20% of the patch and made my first commit with it (r1129909). My main edit to this part (which we may follow in the remaining parts) is replacing lazy initialization of the logger in the getter method with eager initialization to NoopJdbcEventLogger in constructor. This appears to be the most safest and non-intrusive way to add logger to most classes.

        In the remaining patch (aside from what git removes automatically when rebasing against my commit), we can also remove changes to DataContext, TransactionThreadTest, UserTransactionTest that are no longer needed with the approach above.

        The next thing I looked at is JdbcAdapter/JdbcActionBuilder/BaseSQLAction, which seems to be the biggest (and the only?) remaining chunk. Maybe we can also perform with NoopJdbcEventLogger setup in constructor to avoid lazy initialization of the logger... This will give us a fully committable QueryLogger port.

        After that we may further refine it to perfection. The next pass (meaning a separate patch), we may avoid using "setJdbcEventLogger" and use safer constructor injection instead. This will require deeper refactoring so I don't want that to get mixed with any unrelated things.

        Show
        Andrus Adamchik added a comment - Thanks for the patch. Having it all in one piece is a bit hard to process, so I tried to separate into manageable pieces. I succeeded somewhat to split somewhat unrelated 20% of the patch and made my first commit with it (r1129909). My main edit to this part (which we may follow in the remaining parts) is replacing lazy initialization of the logger in the getter method with eager initialization to NoopJdbcEventLogger in constructor. This appears to be the most safest and non-intrusive way to add logger to most classes. In the remaining patch (aside from what git removes automatically when rebasing against my commit), we can also remove changes to DataContext, TransactionThreadTest, UserTransactionTest that are no longer needed with the approach above. The next thing I looked at is JdbcAdapter/JdbcActionBuilder/BaseSQLAction, which seems to be the biggest (and the only?) remaining chunk. Maybe we can also perform with NoopJdbcEventLogger setup in constructor to avoid lazy initialization of the logger... This will give us a fully committable QueryLogger port. After that we may further refine it to perfection. The next pass (meaning a separate patch), we may avoid using "setJdbcEventLogger" and use safer constructor injection instead. This will require deeper refactoring so I don't want that to get mixed with any unrelated things.
        Dzmitry Kazimirchyk made changes -
        Attachment query-logger.patch [ 12480906 ]
        Dzmitry Kazimirchyk made changes -
        Field Original Value New Value
        Attachment query-logger.patch [ 12480832 ]
        Dzmitry Kazimirchyk created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Dzmitry Kazimirchyk
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development