Flume
  1. Flume
  2. FLUME-2959

Fix issues with flume-checkstyle module

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: v1.7.0
    • Component/s: None
    • Labels:
      None

      Description

      In FLUME-2937 we added the flume-checkstyle module to Flume.

      There are three minor issues with it:

      1. It does not follow our modules naming convention. It should be flume-ng-checkstyle.
      2. It does not declare a parent module, resulting in build warnings (the source encoding is not explicitly set to UTF-8)
      3. Resource files don't include the ASF licensing.
      1. FLUME-2959-0.patch
        30 kB
        Lior Zeno
      2. FLUME-2959-1.patch
        3 kB
        Lior Zeno

        Activity

        Hide
        Mike Percy added a comment -

        Thanks for the patch, Lior. Some review feedback:

        1. I don't think the -ng stuff is really helpful and I don't think we need to change it. We do already have flume-tools that is missing the -ng suffix so it's not unprecedented.
        2. The POM should not have flume-parent as a parent because then it would have itself as a dependency which is strange. Maybe we should just specify UTF-8 as the encoding if that is the problem.
        3. Good point, the ASL2.0 license should be included at the top of the XML files.

        Show
        Mike Percy added a comment - Thanks for the patch, Lior. Some review feedback: 1. I don't think the -ng stuff is really helpful and I don't think we need to change it. We do already have flume-tools that is missing the -ng suffix so it's not unprecedented. 2. The POM should not have flume-parent as a parent because then it would have itself as a dependency which is strange. Maybe we should just specify UTF-8 as the encoding if that is the problem. 3. Good point, the ASL2.0 license should be included at the top of the XML files.
        Hide
        Lior Zeno added a comment -

        I don't think the -ng stuff is really helpful and I don't think we need to change it. We do already have flume-tools that is missing the -ng suffix so it's not unprecedented.

        I'm not a big fan of the flume-ng prefix either, but I do think we should have a consistent module naming convention.

        The POM should not have flume-parent as a parent because then it would have itself as a dependency which is strange. Maybe we should just specify UTF-8 as the encoding if that is the problem.

        I've submitted a new patch.

        Thanks.

        Show
        Lior Zeno added a comment - I don't think the -ng stuff is really helpful and I don't think we need to change it. We do already have flume-tools that is missing the -ng suffix so it's not unprecedented. I'm not a big fan of the flume-ng prefix either, but I do think we should have a consistent module naming convention. The POM should not have flume-parent as a parent because then it would have itself as a dependency which is strange. Maybe we should just specify UTF-8 as the encoding if that is the problem. I've submitted a new patch. Thanks.
        Hide
        Mike Percy added a comment -

        I'm not a big fan of the flume-ng prefix either, but I do think we should have a consistent module naming convention.

        Yeah, I agree that maintaining consistency often helps understandability. I also think we should stop doing things when they aren't useful and IMHO this is one of those things.

        Thanks for figuring out why the build was complaining, the patch looks good.

        +1

        Show
        Mike Percy added a comment - I'm not a big fan of the flume-ng prefix either, but I do think we should have a consistent module naming convention. Yeah, I agree that maintaining consistency often helps understandability. I also think we should stop doing things when they aren't useful and IMHO this is one of those things. Thanks for figuring out why the build was complaining, the patch looks good. +1
        Hide
        ASF subversion and git services added a comment -

        Commit 988ede948ffaf6526c226323a6808922f38b625c in flume's branch refs/heads/trunk from Lior Zeno
        [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=988ede9 ]

        FLUME-2959. Fix issues with flume-checkstyle module

        • The lack of a parent POM caused warnings related to UTF-8 encoding
        • The XML files should have an ASL 2.0 license header

        (Lior Zeno via Mike Percy)

        Show
        ASF subversion and git services added a comment - Commit 988ede948ffaf6526c226323a6808922f38b625c in flume's branch refs/heads/trunk from Lior Zeno [ https://git-wip-us.apache.org/repos/asf?p=flume.git;h=988ede9 ] FLUME-2959 . Fix issues with flume-checkstyle module The lack of a parent POM caused warnings related to UTF-8 encoding The XML files should have an ASL 2.0 license header (Lior Zeno via Mike Percy)
        Hide
        Mike Percy added a comment -

        Pushed to trunk. Thanks for the patch Lior!

        Show
        Mike Percy added a comment - Pushed to trunk. Thanks for the patch Lior!
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Flume-trunk-hbase-1 #186 (See https://builds.apache.org/job/Flume-trunk-hbase-1/186/)
        FLUME-2959. Fix issues with flume-checkstyle module (mpercy: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=988ede948ffaf6526c226323a6808922f38b625c)

        • flume-checkstyle/pom.xml
        • flume-checkstyle/src/main/resources/flume/checkstyle-suppressions.xml
        • flume-checkstyle/src/main/resources/flume/checkstyle.xml
        Show
        Hudson added a comment - FAILURE: Integrated in Flume-trunk-hbase-1 #186 (See https://builds.apache.org/job/Flume-trunk-hbase-1/186/ ) FLUME-2959 . Fix issues with flume-checkstyle module (mpercy: http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=988ede948ffaf6526c226323a6808922f38b625c ) flume-checkstyle/pom.xml flume-checkstyle/src/main/resources/flume/checkstyle-suppressions.xml flume-checkstyle/src/main/resources/flume/checkstyle.xml
        Hide
        Lior Zeno added a comment -

        Mike Percy, thanks for committing this. Do you understand why the build has failed?
        I must say that I think something is wrong with the jenkins server, we have had multiple weird build failures recently, each time with a unique unexplained error.

        Show
        Lior Zeno added a comment - Mike Percy , thanks for committing this. Do you understand why the build has failed? I must say that I think something is wrong with the jenkins server, we have had multiple weird build failures recently, each time with a unique unexplained error.
        Hide
        Mike Percy added a comment -

        It looks like general Jenkins slave flakiness to me. Really hard to diagnose with my level of access (maybe there was a network hiccup, or one of the slaves has a bad disk, or someone restarted a process). I looked at a few other builds and it appears that we are using the same build slave pool as them (the label is "ubuntu"). I just kicked off another build, maybe it will succeed this time.

        Show
        Mike Percy added a comment - It looks like general Jenkins slave flakiness to me. Really hard to diagnose with my level of access (maybe there was a network hiccup, or one of the slaves has a bad disk, or someone restarted a process). I looked at a few other builds and it appears that we are using the same build slave pool as them (the label is "ubuntu"). I just kicked off another build, maybe it will succeed this time.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development