Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Fix Version/s: 2.1.1
    • Component/s: None
    • Labels:
      None

      Description

      antlr 3.2 has a problem with java 8, as described here: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8015656

      updating to antlr 3.5.2 solves this, however they have split up the jars differently, which adds some changes, but also the generation of CqlParser.java causes a method to be too large, so i needed to split that method to reduce the size of it.

      (patch against trunk)

      1. cassandra-2.1-7028.txt
        13 kB
        Branimir Lambov
      2. cassandra-2.1-7028.patch
        2.70 MB
        Branimir Lambov
      3. 7028.txt
        11 kB
        Dave Brosius
      4. 7028_v5.patch
        2.70 MB
        Joshua McKenzie
      5. 7028_v4.txt
        11 kB
        Dave Brosius
      6. 7028_v3.txt
        11 kB
        Dave Brosius
      7. 7028_v2.txt
        7 kB
        Joshua McKenzie

        Issue Links

          Activity

          Hide
          cowardlydragon Constance Eustace added a comment -

          I apologize for not actually contributing to the subject at hand, but I can't help myself: I have never seen a library that causes more problems as a dependency than antlr.

          Show
          cowardlydragon Constance Eustace added a comment - I apologize for not actually contributing to the subject at hand, but I can't help myself: I have never seen a library that causes more problems as a dependency than antlr.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          2.1 looks good - committed. Charles Cao: since development focus has shifted to the 2.1 and 3.0 branches at this point, I'm of a mind to hold off on switching out the libraries required to get C* to compile under jdk8 on the 2.0 branch unless there's a big push for it.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - 2.1 looks good - committed. Charles Cao : since development focus has shifted to the 2.1 and 3.0 branches at this point, I'm of a mind to hold off on switching out the libraries required to get C* to compile under jdk8 on the 2.0 branch unless there's a big push for it.
          Hide
          blambov Branimir Lambov added a comment -

          Attached a resynced version of the patch for 2.1. Comparison viewable here.

          Show
          blambov Branimir Lambov added a comment - Attached a resynced version of the patch for 2.1. Comparison viewable here .
          Hide
          PuerTea Charles Cao added a comment -

          Can we also compile and run C* 2.0.x under JDK 8?

          Show
          PuerTea Charles Cao added a comment - Can we also compile and run C* 2.0.x under JDK 8?
          Hide
          tupshin Tupshin Harper added a comment - - edited

          Re-opening and adding additional 2.1.1 target for Aleksey Yashchenko

          Show
          tupshin Tupshin Harper added a comment - - edited Re-opening and adding additional 2.1.1 target for Aleksey Yashchenko
          Hide
          enigmacurry Ryan McGuire added a comment -

          Is it possible to backport this 2.1? Even if there's a reason we don't want to include it in the released version, it would be good to apply the patch and start testing it on cassci.

          I tried wiggle and couldn't get it to apply.

          Show
          enigmacurry Ryan McGuire added a comment - Is it possible to backport this 2.1? Even if there's a reason we don't want to include it in the released version, it would be good to apply the patch and start testing it on cassci. I tried wiggle and couldn't get it to apply.
          Hide
          dbrosius Dave Brosius added a comment -

          committed to trunk as 4d0691759a19f1faafe889d765145ae6a5096397

          Show
          dbrosius Dave Brosius added a comment - committed to trunk as 4d0691759a19f1faafe889d765145ae6a5096397
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Ah - good call on the runtime libraries.

          v4 lost the full index so file deletion failed and the diff has references to the new .jar files which prevents it applying both with the files and without. I've attached a v5 that cleans up some whitespace complaints and includes the binary, both deletion and addition. We should be able to just apply this to trunk and get all the changes - one-shot, no need to download libraries separately and place them for the committer.

          The diff syntax I used to build this was 'git diff --full-index --binary <commit1> <commit2>'. Even w/full-index, if you don't include the binary flag it won't generate the data that goes with your new files you've added and you end up with an invalid patch as it has markers to add files but no binary data to place in them.

          I reran tests on linux against this just to confirm changes to resolve HintedHandOffTest didn't munge with anything else and it all looks good on jdk7. I'm +1 on the v5 patch; give it a run against trunk and let me know how it works for you.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Ah - good call on the runtime libraries. v4 lost the full index so file deletion failed and the diff has references to the new .jar files which prevents it applying both with the files and without. I've attached a v5 that cleans up some whitespace complaints and includes the binary, both deletion and addition. We should be able to just apply this to trunk and get all the changes - one-shot, no need to download libraries separately and place them for the committer. The diff syntax I used to build this was 'git diff --full-index --binary <commit1> <commit2>'. Even w/full-index, if you don't include the binary flag it won't generate the data that goes with your new files you've added and you end up with an invalid patch as it has markers to add files but no binary data to place in them. I reran tests on linux against this just to confirm changes to resolve HintedHandOffTest didn't munge with anything else and it all looks good on jdk7. I'm +1 on the v5 patch; give it a run against trunk and let me know how it works for you.
          Hide
          dbrosius Dave Brosius added a comment - - edited

          pushed a v4, with same changes as v3, but try to get a clean diff file (better git diff options?)

          Show
          dbrosius Dave Brosius added a comment - - edited pushed a v4, with same changes as v3, but try to get a clean diff file (better git diff options?)
          Hide
          dbrosius Dave Brosius added a comment -

          My understanding is that jars that are needed to run cassandra must be committed into the repo in c*/lib. While I agree that seems silly, that's the way it is, i think for making sure we don't forget about licenses or whatnot.

          Show
          dbrosius Dave Brosius added a comment - My understanding is that jars that are needed to run cassandra must be committed into the repo in c*/lib. While I agree that seems silly, that's the way it is, i think for making sure we don't forget about licenses or whatnot.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Actually - v3 doesn't appear to apply to a clean repo clone on trunk:

          errors
          error: missing binary patch data for 'lib/antlr-runtime-3.5.2.jar'
          error: binary patch does not apply to 'lib/antlr-runtime-3.5.2.jar'
          error: lib/antlr-runtime-3.5.2.jar: patch does not apply
          error: missing binary patch data for 'lib/stringtemplate-4.0.2.jar'
          error: binary patch does not apply to 'lib/stringtemplate-4.0.2.jar'
          error: lib/stringtemplate-4.0.2.jar: patch does not apply
          

          The patch works on a repo I was mucking around with but on a clean clone, it's looking for the files that aren't included.

          This goes back to my earlier question - can we have these pulled down the way I had in the _v2 patch and have them read from build/lib, or is there something I'm missing?

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Actually - v3 doesn't appear to apply to a clean repo clone on trunk: errors error: missing binary patch data for 'lib/antlr-runtime-3.5.2.jar' error: binary patch does not apply to 'lib/antlr-runtime-3.5.2.jar' error: lib/antlr-runtime-3.5.2.jar: patch does not apply error: missing binary patch data for 'lib/stringtemplate-4.0.2.jar' error: binary patch does not apply to 'lib/stringtemplate-4.0.2.jar' error: lib/stringtemplate-4.0.2.jar: patch does not apply The patch works on a repo I was mucking around with but on a clean clone, it's looking for the files that aren't included. This goes back to my earlier question - can we have these pulled down the way I had in the _v2 patch and have them read from build/lib, or is there something I'm missing?
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited

          Tests pass, looks good. +1 from me though I do have 1 question:

          What's our reasoning as to what libraries we store in

          {basedir}/lib and what libraries we expect to pull using ant into build/lib/*? antlr was previously housed in {basedir}

          /lib and it looks like you put the antlr runtime and stringtemplate into that folder, however the antlr full is pulled into build/lib. Any insight into the reasoning there?

          Edit: just saw your comment. I think you can include via --binary but yeah - that comes out to over 2MB. If we just had the ant process dnload these libs into build/lib/jars and referenced those during the build, we wouldn't have to worry about manual copying around, no?

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited Tests pass, looks good. +1 from me though I do have 1 question: What's our reasoning as to what libraries we store in {basedir}/lib and what libraries we expect to pull using ant into build/lib/*? antlr was previously housed in {basedir} /lib and it looks like you put the antlr runtime and stringtemplate into that folder, however the antlr full is pulled into build/lib. Any insight into the reasoning there? Edit: just saw your comment. I think you can include via --binary but yeah - that comes out to over 2MB. If we just had the ant process dnload these libs into build/lib/jars and referenced those during the build, we wouldn't have to worry about manual copying around, no?
          Hide
          dbrosius Dave Brosius added a comment -

          and yes, sorry, i should have been more explanatory about the jars.

          after applying the patch, you need to grab antlr-runtime-3.5.2.jar and stringtemplate-4.0.2.jar and paste them into c*/lib, given the way that c* "doesn't" do dependency management.

          perhaps there's a way to include the binary diff of a jar in a diff file, but that would be one large diff file .

          Show
          dbrosius Dave Brosius added a comment - and yes, sorry, i should have been more explanatory about the jars. after applying the patch, you need to grab antlr-runtime-3.5.2.jar and stringtemplate-4.0.2.jar and paste them into c*/lib, given the way that c* "doesn't" do dependency management. perhaps there's a way to include the binary diff of a jar in a diff file, but that would be one large diff file .
          Hide
          dbrosius Dave Brosius added a comment -

          Thanks, nice catch.

          Attached v3

          Show
          dbrosius Dave Brosius added a comment - Thanks, nice catch. Attached v3
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited

          Just confirmed - HintedHandOffTest also fails when linked against stringtemplate-4.0.2.jar, jdk 7 on linux. Dave - Cql's generation / lexing / parsing isn't my area of expertise (yet) - do you want to look into that further and/or confirm you're seeing the same results with your patch locally?

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited Just confirmed - HintedHandOffTest also fails when linked against stringtemplate-4.0.2.jar, jdk 7 on linux. Dave - Cql's generation / lexing / parsing isn't my area of expertise (yet) - do you want to look into that further and/or confirm you're seeing the same results with your patch locally?
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          There were a few things I ironed out: created patch with --full-index, moved the build.lib/* references to build.dir.lib/jars as this patch didn't have a binary add of the {basedir}/lib/*.jar changes, changed from stringtemplate to ST4 as antlr 3.5.2 wouldn't build without it, though the pom for it looks like it references both ST4 and stringtemplate in retrospect... hm.

          Attaching updated patch. All that being said - this patch looks like it breaks HintedHandoff when tested against jdk 7 on linux:

          HintedHandOffTest
              [junit] Error validating UPDATE system.peer_events USING TTL 2592000 SET hints_dropped[ b666cce0-c413-11e3-b7c0-0f1b1496192d ] = 99 WHERE peer = '192.168.1.51'
              [junit] java.lang.RuntimeException: Error validating UPDATE system.peer_events USING TTL 2592000 SET hints_dropped[ b666cce0-c413-11e3-b7c0-0f1b1496192d ] = 99 WHERE peer = '192.168.1.51'
              [junit]     at org.apache.cassandra.cql3.QueryProcessor.processInternal(QueryProcessor.java:211)
              [junit]     at org.apache.cassandra.db.SystemKeyspace.updateHintsDropped(SystemKeyspace.java:406)
              [junit]     at org.apache.cassandra.metrics.HintedHandoffMetrics.log(HintedHandoffMetrics.java:80)
              [junit]     at org.apache.cassandra.db.HintedHandOffTest.testHintsMetrics(HintedHandOffTest.java:90)
              [junit] Caused by: org.apache.cassandra.exceptions.SyntaxException: line 1:102 mismatched input '=' expecting K_WHERE
              [junit]     at org.apache.cassandra.cql3.CqlParser.throwLastRecognitionError(CqlParser.java:281)
              [junit]     at org.apache.cassandra.cql3.QueryProcessor.parseStatement(QueryProcessor.java:349)
              [junit]     at org.apache.cassandra.cql3.QueryProcessor.getStatement(QueryProcessor.java:323)
              [junit]     at org.apache.cassandra.cql3.QueryProcessor.processInternal(QueryProcessor.java:197)
              [junit]
              [junit]
              [junit] Test org.apache.cassandra.db.HintedHandOffTest FAILED
          

          So I'm a -1 until we can get that figured out. May very well be due to ST4 ant / maven package changes, so I'll take a look. Also - we'll need to go for a v3 at least if we go ST4 as I forgot to update license from 4.0.2 to 4.0.8.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - There were a few things I ironed out: created patch with --full-index, moved the build.lib/* references to build.dir.lib/jars as this patch didn't have a binary add of the {basedir}/lib/*.jar changes, changed from stringtemplate to ST4 as antlr 3.5.2 wouldn't build without it, though the pom for it looks like it references both ST4 and stringtemplate in retrospect... hm. Attaching updated patch. All that being said - this patch looks like it breaks HintedHandoff when tested against jdk 7 on linux: HintedHandOffTest [junit] Error validating UPDATE system.peer_events USING TTL 2592000 SET hints_dropped[ b666cce0-c413-11e3-b7c0-0f1b1496192d ] = 99 WHERE peer = '192.168.1.51' [junit] java.lang.RuntimeException: Error validating UPDATE system.peer_events USING TTL 2592000 SET hints_dropped[ b666cce0-c413-11e3-b7c0-0f1b1496192d ] = 99 WHERE peer = '192.168.1.51' [junit] at org.apache.cassandra.cql3.QueryProcessor.processInternal(QueryProcessor.java:211) [junit] at org.apache.cassandra.db.SystemKeyspace.updateHintsDropped(SystemKeyspace.java:406) [junit] at org.apache.cassandra.metrics.HintedHandoffMetrics.log(HintedHandoffMetrics.java:80) [junit] at org.apache.cassandra.db.HintedHandOffTest.testHintsMetrics(HintedHandOffTest.java:90) [junit] Caused by: org.apache.cassandra.exceptions.SyntaxException: line 1:102 mismatched input '=' expecting K_WHERE [junit] at org.apache.cassandra.cql3.CqlParser.throwLastRecognitionError(CqlParser.java:281) [junit] at org.apache.cassandra.cql3.QueryProcessor.parseStatement(QueryProcessor.java:349) [junit] at org.apache.cassandra.cql3.QueryProcessor.getStatement(QueryProcessor.java:323) [junit] at org.apache.cassandra.cql3.QueryProcessor.processInternal(QueryProcessor.java:197) [junit] [junit] [junit] Test org.apache.cassandra.db.HintedHandOffTest FAILED So I'm a -1 until we can get that figured out. May very well be due to ST4 ant / maven package changes, so I'll take a look. Also - we'll need to go for a v3 at least if we go ST4 as I forgot to update license from 4.0.2 to 4.0.8.
          Hide
          jbellis Jonathan Ellis added a comment -

          Joshua McKenzie to review

          Show
          jbellis Jonathan Ellis added a comment - Joshua McKenzie to review

            People

            • Assignee:
              blambov Branimir Lambov
              Reporter:
              dbrosius Dave Brosius
              Reviewer:
              Joshua McKenzie
            • Votes:
              0 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development