Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: 1.1.0
    • Component/s: blueprints
    • Labels:
      None

      Description

      Weatherman is a stochastic model for weather simulation. I had built it using components from BigPetStore's data generator.

      https://github.com/bigpetstore/bigpetstore-weather-generator

      Should be imported into BigTop Data Generators

      1. BIGTOP-1991.2.patch
        205 kB
        RJ Nowling
      2. BIGTOP-1991.3.patch
        565 kB
        RJ Nowling
      3. BIGTOP-1991.4.patch
        232 kB
        RJ Nowling
      4. BIGTOP-1991.patch
        198 kB
        RJ Nowling

        Activity

        Hide
        rnowling RJ Nowling added a comment -

        Adds BigTop Weatherman under the BigTop Data Generators

        I'm gonna leave this here until the community okays further progress on BigTop Data Generators

        Show
        rnowling RJ Nowling added a comment - Adds BigTop Weatherman under the BigTop Data Generators I'm gonna leave this here until the community okays further progress on BigTop Data Generators
        Hide
        rnowling RJ Nowling added a comment -

        Updated patch updates build instructions for BigTop Weatherman and the other BigTop data generators.

        Show
        rnowling RJ Nowling added a comment - Updated patch updates build instructions for BigTop Weatherman and the other BigTop data generators.
        Hide
        cos Konstantin Boudnik added a comment -

        Some quick comments (will look more into it):

        • applying the patch produces a lot of whitespace errors {verbatim}
          Applying: BIGTOP-1991. Add BigTop Weatherman
          /home/cos/workspaces/bigtop/.git/rebase-apply/patch:173: trailing whitespace.

          /home/cos/workspaces/bigtop/.git/rebase-apply/patch:183: trailing whitespace.

          /home/cos/workspaces/bigtop/.git/rebase-apply/patch:301: trailing whitespace.

          /home/cos/workspaces/bigtop/.git/rebase-apply/patch:380: trailing whitespace.
          apply plugin: 'java'
          /home/cos/workspaces/bigtop/.git/rebase-apply/patch:386: trailing whitespace.

          warning: squelched 101 whitespace errors
          warning: 106 lines add whitespace errors.{verbatim}
        • Java code has non-conventional placement of the '{' symbols - they have to be on the same line where the block starts, not the following line as in C
        • initialization of the file in the constant container might back-fire if the file isn't presented
          public static final File WEATHER_PARAMETERS_FILE = new File("weather_parameters.csv");

          Perhaps the logic is better be moved to where this constant is used and WEATHER_PARAMETERS_FILE should only contain the name of the file, which can possibly be overridden in the runtime? Also, considering that the provided sample file sits under resources directory you might want to consider loading it from the classloader resources instead.

        • some JavaDocs would be nice if these are the APIs intended for downstream use
        • gradle build has explicit version string. This is error-prone when new releases are cut as more places need to be checked for the version change. is there a way to use the top level version everywhere?
        Show
        cos Konstantin Boudnik added a comment - Some quick comments (will look more into it): applying the patch produces a lot of whitespace errors {verbatim} Applying: BIGTOP-1991 . Add BigTop Weatherman /home/cos/workspaces/bigtop/.git/rebase-apply/patch:173: trailing whitespace. /home/cos/workspaces/bigtop/.git/rebase-apply/patch:183: trailing whitespace. /home/cos/workspaces/bigtop/.git/rebase-apply/patch:301: trailing whitespace. /home/cos/workspaces/bigtop/.git/rebase-apply/patch:380: trailing whitespace. apply plugin: 'java' /home/cos/workspaces/bigtop/.git/rebase-apply/patch:386: trailing whitespace. warning: squelched 101 whitespace errors warning: 106 lines add whitespace errors.{verbatim} Java code has non-conventional placement of the '{' symbols - they have to be on the same line where the block starts, not the following line as in C initialization of the file in the constant container might back-fire if the file isn't presented public static final File WEATHER_PARAMETERS_FILE = new File( "weather_parameters.csv" ); Perhaps the logic is better be moved to where this constant is used and WEATHER_PARAMETERS_FILE should only contain the name of the file, which can possibly be overridden in the runtime? Also, considering that the provided sample file sits under resources directory you might want to consider loading it from the classloader resources instead. some JavaDocs would be nice if these are the APIs intended for downstream use gradle build has explicit version string. This is error-prone when new releases are cut as more places need to be checked for the version change. is there a way to use the top level version everywhere?
        Hide
        rnowling RJ Nowling added a comment -

        Thanks, Konstantin Boudnik! I'll make the appropriate changes.

        Show
        rnowling RJ Nowling added a comment - Thanks, Konstantin Boudnik ! I'll make the appropriate changes.
        Hide
        rnowling RJ Nowling added a comment -

        Hi Konstantin Boudnik,

        I uploaded a new patch which:

        1. Fixes whitespace errors
        2. Fixed Java code formatting
        3. Adds java docs to the public classes in BigTop Weatherman
        4. Moves redundant code (including the version) from per-project Gradle build files into the common build file
        5. Pulls group id and version from top-level pom.xml.

        I realize that BigTop is moving towards dumping Maven, however the solution for loading the group id and version from the pom.xml might suggest a way to solve the overall problem in BigTop. The group id and version can be set programmatically, so we might be able to write a simple function to read the version from the bigtop.mk file in the future.

        Alternatively, we could add the data generators as a subproject of the top-level Gradle build. However, this would make the data generators required and I feel that goes against the wishes expressed by members of the community. Not to mention, I seem to have to build all of the data generator projects as a cohesive unit now that it's a multi-project build – I can't build them individually. I don't want to complicate that further by tying into the main BigTop Gradle build right now.

        I have not addressed the issue of the hard-coded path yet. I would like to look further into loading class resources and propose a JIRA to update all of the data generator projects to follow the new approach, if we decide to go down that path.

        Thanks for the review!

        Show
        rnowling RJ Nowling added a comment - Hi Konstantin Boudnik , I uploaded a new patch which: 1. Fixes whitespace errors 2. Fixed Java code formatting 3. Adds java docs to the public classes in BigTop Weatherman 4. Moves redundant code (including the version) from per-project Gradle build files into the common build file 5. Pulls group id and version from top-level pom.xml . I realize that BigTop is moving towards dumping Maven, however the solution for loading the group id and version from the pom.xml might suggest a way to solve the overall problem in BigTop. The group id and version can be set programmatically, so we might be able to write a simple function to read the version from the bigtop.mk file in the future. Alternatively, we could add the data generators as a subproject of the top-level Gradle build. However, this would make the data generators required and I feel that goes against the wishes expressed by members of the community. Not to mention, I seem to have to build all of the data generator projects as a cohesive unit now that it's a multi-project build – I can't build them individually. I don't want to complicate that further by tying into the main BigTop Gradle build right now. I have not addressed the issue of the hard-coded path yet. I would like to look further into loading class resources and propose a JIRA to update all of the data generator projects to follow the new approach, if we decide to go down that path. Thanks for the review!
        Hide
        cos Konstantin Boudnik added a comment -

        Thanks for the update - I will review shortly. For now I want to update a couple of points you've expressed:

        I realize that BigTop is moving towards dumping Maven, however the solution for loading the group id and version from the pom.xml might suggest a way to solve the overall problem in BigTop.

        it doesn't mean that we'll get rid of pom files - we still need to deploy artifacts and make them discoverable via maven repo-servers. BTW, bigtop.mk is going away and will be replaced in BIGTOP-1494 by a more friendly DSL (hopefully soon).

        I have not addressed the issue of the hard-coded path yet.

        While hard-coded paths aren't the prettiest thing - they might be ok. My concern is different. Unless you use something like

        this.getClass().getClassLoader().getResource("weather_parameters.csv")
        

        generators' users will have to run these programs in a very specific file contest - otherwise these resource files won't be found. That probably means some sort of scripts to setup said file context making everything more complex and so on.

        Show
        cos Konstantin Boudnik added a comment - Thanks for the update - I will review shortly. For now I want to update a couple of points you've expressed: I realize that BigTop is moving towards dumping Maven, however the solution for loading the group id and version from the pom.xml might suggest a way to solve the overall problem in BigTop. it doesn't mean that we'll get rid of pom files - we still need to deploy artifacts and make them discoverable via maven repo-servers. BTW, bigtop.mk is going away and will be replaced in BIGTOP-1494 by a more friendly DSL (hopefully soon). I have not addressed the issue of the hard-coded path yet. While hard-coded paths aren't the prettiest thing - they might be ok. My concern is different. Unless you use something like this .getClass().getClassLoader().getResource( "weather_parameters.csv" ) generators' users will have to run these programs in a very specific file contest - otherwise these resource files won't be found. That probably means some sort of scripts to setup said file context making everything more complex and so on.
        Hide
        cos Konstantin Boudnik added a comment - - edited

        Sorry for being a PITA, but I still see the formatting issues

        • with '{' on the separate line
        • indents are 8 whitespaces in java code; and 4 whitespace in the gradle
          We pretty much do follow same style guide as Java with a couple modifications
        • 2 spaces for indents, and 4 for lines continuations
        • trying to cap the lines at 80 chars (this one is a sort of nice to have
          Could you please fix these?

        As a side thought: perhaps it would make sense to be producing a soft-link for the versioned jar file, otherwise we'll have to keep updating the documentation for things like

        java -jar build/libs/bigpetstore-data-generator-1.1.0-SNAPSHOT.jar
        

        Or may be just replace the version with <VERSION> macro or something?

        Show
        cos Konstantin Boudnik added a comment - - edited Sorry for being a PITA, but I still see the formatting issues with '{' on the separate line indents are 8 whitespaces in java code; and 4 whitespace in the gradle We pretty much do follow same style guide as Java with a couple modifications 2 spaces for indents, and 4 for lines continuations trying to cap the lines at 80 chars (this one is a sort of nice to have Could you please fix these? As a side thought: perhaps it would make sense to be producing a soft-link for the versioned jar file, otherwise we'll have to keep updating the documentation for things like java -jar build/libs/bigpetstore-data-generator-1.1.0-SNAPSHOT.jar Or may be just replace the version with <VERSION> macro or something?
        Hide
        rnowling RJ Nowling added a comment - - edited

        Thanks, Konstantin Boudnik. No worries about attention to detail – always a good thing!

        For the formatting, I changed my Eclipse settings and reformatted the code but it seems I messed something up when creating the patch. I'll make the original changes you requested plus the spacing and line length settings. Sorry for not seeing that and asking you to review that twice!

        For the parameters file, I think we're doing what you're saying. In WeatherParametersReader, we do the following:

        getClass().getResourceAsStream("/input_data/" + filename);
        

        where the filename comes from reading the static variable defined in the constants class. You had originally suggested moving the constant to the class where it's used – I can do that if that satisfies the issue.

        Thanks!

        Show
        rnowling RJ Nowling added a comment - - edited Thanks, Konstantin Boudnik . No worries about attention to detail – always a good thing! For the formatting, I changed my Eclipse settings and reformatted the code but it seems I messed something up when creating the patch. I'll make the original changes you requested plus the spacing and line length settings. Sorry for not seeing that and asking you to review that twice! For the parameters file, I think we're doing what you're saying. In WeatherParametersReader , we do the following: getClass().getResourceAsStream( "/input_data/" + filename); where the filename comes from reading the static variable defined in the constants class. You had originally suggested moving the constant to the class where it's used – I can do that if that satisfies the issue. Thanks!
        Hide
        rnowling RJ Nowling added a comment -

        Hi Konstantin Boudnik,

        I've attached a new patch. The old patch was really 10 separate commits in one patch so it wasn't obvious that I had reformatted the code, added Java docs, etc. – you'd have to scroll through all 10 commits! This time, I rebased and squashed the commits.

        I updated all of the Gradle build files and Java code to use 2 spaces for indentation. I also set the Java code to wrap at 80 lines. Note that I only reformatted Java code directly affected by the patch – I did not attempt to reformat code outside the functional scope of the patch. I'm going to combine those in later JIRAs to avoid making huge patches that are hard to review.

        I haven't made all the other changes (e.g., moving the parameters file constant) or adding a symlink to the latest jar. I want to focus on getting the code formatting correct and then follow up with subsequent JIRAs on the other issues.

        Please let me know if this works – if not, I'm happy to make further changes.

        RJ

        Show
        rnowling RJ Nowling added a comment - Hi Konstantin Boudnik , I've attached a new patch. The old patch was really 10 separate commits in one patch so it wasn't obvious that I had reformatted the code, added Java docs, etc. – you'd have to scroll through all 10 commits! This time, I rebased and squashed the commits. I updated all of the Gradle build files and Java code to use 2 spaces for indentation. I also set the Java code to wrap at 80 lines. Note that I only reformatted Java code directly affected by the patch – I did not attempt to reformat code outside the functional scope of the patch. I'm going to combine those in later JIRAs to avoid making huge patches that are hard to review. I haven't made all the other changes (e.g., moving the parameters file constant) or adding a symlink to the latest jar. I want to focus on getting the code formatting correct and then follow up with subsequent JIRAs on the other issues. Please let me know if this works – if not, I'm happy to make further changes. RJ
        Hide
        rnowling RJ Nowling added a comment -

        Hey Konstantin Boudnik,

        Could you review the latest patch when you have time?

        Thanks!
        RJ

        Show
        rnowling RJ Nowling added a comment - Hey Konstantin Boudnik , Could you review the latest patch when you have time? Thanks! RJ
        Hide
        cos Konstantin Boudnik added a comment -

        Sorry about the delay RJ...
        The patch looks good now. There are still a couple of places in README files where the lines are >80, but it isn't big of a deal: let's get it committed so you can move forward with this. Thanks!

        +1

        Show
        cos Konstantin Boudnik added a comment - Sorry about the delay RJ... The patch looks good now. There are still a couple of places in README files where the lines are >80, but it isn't big of a deal: let's get it committed so you can move forward with this. Thanks! +1
        Hide
        rnowling RJ Nowling added a comment -

        Committed!

        Thank you for the review, Konstantin Boudnik!

        Show
        rnowling RJ Nowling added a comment - Committed! Thank you for the review, Konstantin Boudnik !

          People

          • Assignee:
            rnowling RJ Nowling
            Reporter:
            rnowling RJ Nowling
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development