ODE
  1. ODE
  2. ODE-626

Unpack details blob from ODE_JOB table

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 1.3.4, 2.0
    • Component/s: BPEL Runtime
    • Labels:
      None

      Description

      Related discussion is here:
      http://markmail.org/thread/iiy6utvsqew6nn6m

        Activity

        Hide
        Rafal Rusin added a comment -

        I ended up with quite large changes. Work is almost done (it passes all test cases), but I've got a few questions about cron scheduling implementation yet.

        I extracted entries for:
        + instanceId BIGINT,
        + mexId varchar(255),
        + processId varchar(255),
        + type varchar(255),
        + channel varchar(255),
        + correlatorId varchar(255),
        + correlationKey varchar(255),
        + retryCount int,
        + inMem int,
        + detailsExt blob,

        I needed to hold blob column - detailsExt, which is used for instance cleanup. It uses columns cleanupInfo, pidsToExclude, pid and trasactionTimeout. I believe it's convenient to leave such blob column for various extensions. If extensions map is empty, then it's serialized to null in DB.

        However, I've got 2 issues.

        • I needed to put JobDetailsImpl to bpel-iapi, because job details are instantiated inside ProcessConfImpl. I'd like to add factory method createDetails into Scheduler bpel-api and add scheduler dependency to ProcessConfImpl, but please confirm it.
        • I left pid column in cleanup runnables (ProcessCleanUpRunnable, RuntimeDataCleanupRunnable) inside detailsExt blob, but please confirm if I can change constructor
          public ProcessCleanUpRunnable(Serializable pid)
          with _pid to QName and pass toString() here:
          _contexts.dao.getConnection().createTransientProcess(_pid.toString())

        OK, it's all.
        Thanks in advance for reply.

        Show
        Rafal Rusin added a comment - I ended up with quite large changes. Work is almost done (it passes all test cases), but I've got a few questions about cron scheduling implementation yet. I extracted entries for: + instanceId BIGINT, + mexId varchar(255), + processId varchar(255), + type varchar(255), + channel varchar(255), + correlatorId varchar(255), + correlationKey varchar(255), + retryCount int, + inMem int, + detailsExt blob, I needed to hold blob column - detailsExt, which is used for instance cleanup. It uses columns cleanupInfo, pidsToExclude, pid and trasactionTimeout. I believe it's convenient to leave such blob column for various extensions. If extensions map is empty, then it's serialized to null in DB. However, I've got 2 issues. I needed to put JobDetailsImpl to bpel-iapi, because job details are instantiated inside ProcessConfImpl. I'd like to add factory method createDetails into Scheduler bpel-api and add scheduler dependency to ProcessConfImpl, but please confirm it. I left pid column in cleanup runnables (ProcessCleanUpRunnable, RuntimeDataCleanupRunnable) inside detailsExt blob, but please confirm if I can change constructor public ProcessCleanUpRunnable(Serializable pid) with _pid to QName and pass toString() here: _contexts.dao.getConnection().createTransientProcess(_pid.toString()) OK, it's all. Thanks in advance for reply.
        Hide
        Rafal Rusin added a comment -

        I committed a version without factory method for JobDetails interface.

        Show
        Rafal Rusin added a comment - I committed a version without factory method for JobDetails interface.
        Hide
        Alexis Midon added a comment - - edited

        #1. Regarding an eventual factory method to instantiate JobDetails, I'm wondering why we need an interface in the first place. JodDetails is a pojo and I can't think of any other implementations. I think the interface & factory is trying to solve an issue that does not exist yet, which is not good. So my opinion is: delete the interface JobDetails, rename JobDetailsImpl into JobDetails.

        #2. Regarding ProcessCleanUpRunnable, RuntimeDataCleanupRunnable, I think Sean will know. I'll ping him for feedback.

        #3 why should we keep the WorkEvent class? The purpose of this class was to "wraps around job detail map", witch is what your patch solves. I would remove the WorkEvent class and use JodDetails instead.

        #4 don't you think we could maintain compatibility between old (packed) jobs and the new (unpacked) ones? This could spare a lot of migration pain for existing databases. JdbcDelegate#dequeueImmediate could check the detailsExt map for old keys and assign values to the corresponding new instance attributes.

        Show
        Alexis Midon added a comment - - edited #1. Regarding an eventual factory method to instantiate JobDetails, I'm wondering why we need an interface in the first place. JodDetails is a pojo and I can't think of any other implementations. I think the interface & factory is trying to solve an issue that does not exist yet, which is not good. So my opinion is: delete the interface JobDetails, rename JobDetailsImpl into JobDetails. #2. Regarding ProcessCleanUpRunnable, RuntimeDataCleanupRunnable, I think Sean will know. I'll ping him for feedback. #3 why should we keep the WorkEvent class? The purpose of this class was to "wraps around job detail map", witch is what your patch solves. I would remove the WorkEvent class and use JodDetails instead. #4 don't you think we could maintain compatibility between old (packed) jobs and the new (unpacked) ones? This could spare a lot of migration pain for existing databases. JdbcDelegate#dequeueImmediate could check the detailsExt map for old keys and assign values to the corresponding new instance attributes.
        Hide
        Alexis Midon added a comment -

        it's all yours!

        Show
        Alexis Midon added a comment - it's all yours!
        Hide
        Sean Ahn added a comment -

        Rafal,

        I think you can do any refactoring you want to do. I agree with Alexis on that the code has already become pretty messy around the this job details map.

        Thanks for the refactoring effort.

        Show
        Sean Ahn added a comment - Rafal, I think you can do any refactoring you want to do. I agree with Alexis on that the code has already become pretty messy around the this job details map. Thanks for the refactoring effort.
        Hide
        Rafal Rusin added a comment -

        Thanks for reply.
        As for:
        #1 - suggestion is OK. I just wanted to keep interface separation, because JobDetailsImpl is not strictly POJO. For example is converts JobType enum to String type field or pid QName to String pid field. JdbcDelegate makes use of those public fields. But I agree, it's less mess without separation.
        #3 - I kept it for naming reasons. There's a lot of code, which references it. But I agree it's for removal, so I'll do that.
        #4 - OK, I'll do that. I haven't considered migration yet, because Alex pointed it's not necessary for ODE 2.X, sice it's not in production yet.

        Show
        Rafal Rusin added a comment - Thanks for reply. As for: #1 - suggestion is OK. I just wanted to keep interface separation, because JobDetailsImpl is not strictly POJO. For example is converts JobType enum to String type field or pid QName to String pid field. JdbcDelegate makes use of those public fields. But I agree, it's less mess without separation. #3 - I kept it for naming reasons. There's a lot of code, which references it. But I agree it's for removal, so I'll do that. #4 - OK, I'll do that. I haven't considered migration yet, because Alex pointed it's not necessary for ODE 2.X, sice it's not in production yet.
        Hide
        Rafal Rusin added a comment -

        I addressed issues #1, #3, #4.

        Show
        Rafal Rusin added a comment - I addressed issues #1, #3, #4.
        Hide
        Alexis Midon added a comment -

        great! thanks a lot

        Show
        Alexis Midon added a comment - great! thanks a lot

          People

          • Assignee:
            Rafal Rusin
            Reporter:
            Rafal Rusin
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development