Solr
  1. Solr
  2. SOLR-3329

Don't use svn:properties Id or Revision in SolrInfoMBean

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      In solr, use use svn:keywords haphazardly

      We have lots of places with:

      svn propset svn:keywords "Date Author Id Revision HeadURL" *.java
      

      In LUCENE-3923, there is a suggestion to get rid of many of these.

      The MBeans interface often exposes HeadURL, but we likely want to get rid of the rest

        Issue Links

          Activity

          Hide
          Ryan McKinley added a comment -

          In general, we need to decide what to do for SolrInfoMBean – the current typical implemention is:

          
            @Override
            public String getVersion() {
              return "$Revision: 1306018 $";
            }
          
            @Override
            public String getSourceId() {
              return "$Id: RealTimeGetComponent.java 1306018 2012-03-27 20:58:34Z yonik $";
            }
          
            @Override
            public String getSource() {
              return "$URL: https://svn.apache.org/repos/asf/lucene/dev/trunk/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java $";
            }
          

          but we often forget to set the svn:keyword property when adding new MBeans...

          Show
          Ryan McKinley added a comment - In general, we need to decide what to do for SolrInfoMBean – the current typical implemention is: @Override public String getVersion() { return "$Revision: 1306018 $" ; } @Override public String getSourceId() { return "$Id: RealTimeGetComponent.java 1306018 2012-03-27 20:58:34Z yonik $" ; } @Override public String getSource() { return "$URL: https: //svn.apache.org/repos/asf/lucene/dev/trunk/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java $" ; } but we often forget to set the svn:keyword property when adding new MBeans...
          Hide
          Yonik Seeley added a comment -

          Honestly, I don't think I've ever found that extra info useful...

          Show
          Yonik Seeley added a comment - Honestly, I don't think I've ever found that extra info useful...
          Hide
          Hoss Man added a comment -

          These were all really designed originally for people writting plugins to be able to expose more information for their consumers about them that might not be obvious based on the global info about the Solr intall.

          As for stuff in the solr source tree, i would suggest..

          • getSource() - keep using $URL$, it doesn't really hurt anything.
          • getVersion() - we should just start returning the implementaiton version from the package metadata
          • getSourceId() - $Id$ is the most problematic svn keyword i've ever seen, lets just drop it and leave this blank in all the core mbeans ... plugin writers can use it however they want
          Show
          Hoss Man added a comment - These were all really designed originally for people writting plugins to be able to expose more information for their consumers about them that might not be obvious based on the global info about the Solr intall. As for stuff in the solr source tree, i would suggest.. getSource() - keep using $URL$, it doesn't really hurt anything. getVersion() - we should just start returning the implementaiton version from the package metadata getSourceId() - $Id$ is the most problematic svn keyword i've ever seen, lets just drop it and leave this blank in all the core mbeans ... plugin writers can use it however they want
          Hide
          Chris Male added a comment - - edited

          I agree with Yonik, I've never found this information useful. +1 to Hoss's suggestion.

          Show
          Chris Male added a comment - - edited I agree with Yonik, I've never found this information useful. +1 to Hoss's suggestion.
          Hide
          Ryan McKinley added a comment -

          implements Chris's suggestion and removes "Date Author Id Revision" from most files

          Show
          Ryan McKinley added a comment - implements Chris's suggestion and removes "Date Author Id Revision" from most files
          Hide
          Ryan McKinley added a comment -

          committed in #1310219

          Show
          Ryan McKinley added a comment - committed in #1310219
          Hide
          Robert Muir added a comment -

          keep using $URL$, it doesn't really hurt anything.

          This still hurts when using ordinary patch/diff tools across different branches.

          I frequently do this (I use patch --merge to merge outdated patches, and I use
          diff to show changes including svn adds/deletes).

          For example, i use the dev-tools/scripts/diffSources.py to review the differences
          between a feature branch and trunk before merging it back.

          Show
          Robert Muir added a comment - keep using $URL$, it doesn't really hurt anything. This still hurts when using ordinary patch/diff tools across different branches. I frequently do this (I use patch --merge to merge outdated patches, and I use diff to show changes including svn adds/deletes). For example, i use the dev-tools/scripts/diffSources.py to review the differences between a feature branch and trunk before merging it back.
          Hide
          Ryan McKinley added a comment -

          I'm changing the ticket name so it more accurately reflects the changes.

          Robert.. we can look at the HeadURL issue in a different ticket. I think keeping the URL in the bean is useful – perhaps we just need to remove the property from things that are not MBeans?

          Show
          Ryan McKinley added a comment - I'm changing the ticket name so it more accurately reflects the changes. Robert.. we can look at the HeadURL issue in a different ticket. I think keeping the URL in the bean is useful – perhaps we just need to remove the property from things that are not MBeans?

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Ryan McKinley
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development