Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6381

Unnecessary synchronized object in BucketingSink

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: filesystem-connector
    • Labels:
      None

      Description

      It seems that currently there are two places should not employ the synchronized to describe pendingFilesPerCheckpoint, as it is only restored state object for checkpoint and no sharing of the data-structure between different threads, as follows.

       private void handleRestoredRollingSinkState(RollingSink.BucketState restoredState) {
           ...
          synchronized (restoredState.pendingFilesPerCheckpoint) {
      			restoredState.pendingFilesPerCheckpoint.clear();
      		}
           ...
      }

      and

      private void handleRestoredBucketState(State<T> restoredState) {       
          ...
          synchronized (bucketState.pendingFilesPerCheckpoint) {
      	     	        bucketState.pendingFilesPerCheckpoint.clear();
      		}
       } 

      Hi, Kostas Kloudas. Is there any other stuff shoud add here ? Would you mind have a more thorough look in this class ?

        Issue Links

          Activity

          Hide
          kkl0u Kostas Kloudas added a comment - - edited

          Hi mingleizhang! I updated the Type to improvement, as the locking just seems to be redundant, rather than a BUG that can cause problems.

          Thanks for working on this. I will have a look in the class in the following days.

          Show
          kkl0u Kostas Kloudas added a comment - - edited Hi mingleizhang ! I updated the Type to improvement, as the locking just seems to be redundant, rather than a BUG that can cause problems. Thanks for working on this. I will have a look in the class in the following days.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user zhangminglei opened a pull request:

          https://github.com/apache/flink/pull/3820

          FLINK-6381 [connector] Unnecessary synchronizing object in Bucketin…

          Currently there are two places should not employ the synchronized to describe ```pendingFilesPerCheckpoint```, as it is only restored state object for checkpoint and no sharing of the data-structure between different threads

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/zhangminglei/flink flink-6381

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3820.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3820


          commit ba9be98844d81ddec7cd8aae95457f2e3e3f6acb
          Author: zhangminglei <zml13856086071@163.com>
          Date: 2017-05-04T07:11:20Z

          FLINK-6381 [connector] Unnecessary synchronizing object in BucketingSink.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user zhangminglei opened a pull request: https://github.com/apache/flink/pull/3820 FLINK-6381 [connector] Unnecessary synchronizing object in Bucketin… Currently there are two places should not employ the synchronized to describe ```pendingFilesPerCheckpoint```, as it is only restored state object for checkpoint and no sharing of the data-structure between different threads You can merge this pull request into a Git repository by running: $ git pull https://github.com/zhangminglei/flink flink-6381 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3820.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3820 commit ba9be98844d81ddec7cd8aae95457f2e3e3f6acb Author: zhangminglei <zml13856086071@163.com> Date: 2017-05-04T07:11:20Z FLINK-6381 [connector] Unnecessary synchronizing object in BucketingSink.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei commented on the issue:

          https://github.com/apache/flink/pull/3820

          Hi, @kl0u. It would be great if you can take a look at this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/3820 Hi, @kl0u. It would be great if you can take a look at this PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kl0u commented on the issue:

          https://github.com/apache/flink/pull/3820

          Hi @zhangminglei ! I will have a look during this week. Sorry for the delay.

          Show
          githubbot ASF GitHub Bot added a comment - Github user kl0u commented on the issue: https://github.com/apache/flink/pull/3820 Hi @zhangminglei ! I will have a look during this week. Sorry for the delay.
          Hide
          mingleizhang mingleizhang added a comment - - edited

          Kostas Kloudas Thanks. I guess sometimes it is a tricky problem when refer to synchoronize which objects should own it until it trigger a concurrency issue. What do you think of this ?

          Show
          mingleizhang mingleizhang added a comment - - edited Kostas Kloudas Thanks. I guess sometimes it is a tricky problem when refer to synchoronize which objects should own it until it trigger a concurrency issue. What do you think of this ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kl0u commented on the issue:

          https://github.com/apache/flink/pull/3820

          Thanks for your work @zhangminglei . Changes look good! Merging this.

          Show
          githubbot ASF GitHub Bot added a comment - Github user kl0u commented on the issue: https://github.com/apache/flink/pull/3820 Thanks for your work @zhangminglei . Changes look good! Merging this.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei commented on the issue:

          https://github.com/apache/flink/pull/3820

          Thanks for review @kl0u and the travis had gave us the green light. Very appreciate it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/3820 Thanks for review @kl0u and the travis had gave us the green light. Very appreciate it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kl0u commented on the issue:

          https://github.com/apache/flink/pull/3820

          Merged this. Could you close the PR and the related JIRA?

          Show
          githubbot ASF GitHub Bot added a comment - Github user kl0u commented on the issue: https://github.com/apache/flink/pull/3820 Merged this. Could you close the PR and the related JIRA?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei commented on the issue:

          https://github.com/apache/flink/pull/3820

          @kl0u Okay. I will do this soon. Thanks again.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei commented on the issue: https://github.com/apache/flink/pull/3820 @kl0u Okay. I will do this soon. Thanks again.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhangminglei closed the pull request at:

          https://github.com/apache/flink/pull/3820

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhangminglei closed the pull request at: https://github.com/apache/flink/pull/3820

            People

            • Assignee:
              mingleizhang mingleizhang
              Reporter:
              mingleizhang mingleizhang
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development