Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No hard-coded, sprinkled, 1996! #51

Closed
egh opened this issue Jan 7, 2014 · 14 comments
Closed

No hard-coded, sprinkled, 1996! #51

egh opened this issue Jan 7, 2014 · 14 comments

Comments

@egh
Copy link
Contributor

egh commented Jan 7, 2014

Move to Spring?

@egh
Copy link
Contributor Author

egh commented Jan 8, 2014

Adding interested parties from spreadsheet. @kris-sigur @anjackson @PsypherPunk

@egh
Copy link
Contributor Author

egh commented Jan 14, 2014

@johnerikhalse johnerikhalse added this to the 2.0.1 Release milestone Sep 30, 2014
@RogerMathisen
Copy link
Contributor

I have started to look into this issue, and am trying to figure out the code in Timestamp.java. One of the things that puzzles me is the definition of several MAX MIN limits:

private final static String MONTH_LOWER_LIMIT = "01";
private final static String MONTH_UPPER_LIMIT = "12";
private final static String DAY_LOWER_LIMIT = "01";
private final static String HOUR_UPPER_LIMIT = "23";
private final static String HOUR_LOWER_LIMIT = "00";
private final static String MINUTE_UPPER_LIMIT = "59";
private final static String MINUTE_LOWER_LIMIT = "00";
private final static String SECOND_UPPER_LIMIT = "59";
private final static String SECOND_LOWER_LIMIT = "00";

Does anybody have situations where these variables could be set differently than the above? If not why isn't simply the Calendars getMaximum() and getMinimum() methods used to test whether date strings are legal or not

@RogerMathisen
Copy link
Contributor

One thing to consider when making the start year configurable, is how to configure. So far I have implemented a solution that uses a system parameter to set the default start date from the JVM command line eg: "-Dwayback.timestamp.startyear=1997".

Another possible solution would be to put the configuration in a .properties under /src/main/resources/org/archive/wayback/util (Or perhaps allow for both?).

@kris-sigur
Copy link
Member

It all seems to come down to the boundTimestamp method that is used for validation. It seems to me that this class should really be rewritten so that it uses a suitable utility method from DateUtils to convert the timestamp string into a Date object. Same issue in method dateStrToCalendar.

I guess the good news is that none of these constants are used outside of Timestamp.java.

@kris-sigur
Copy link
Member

On the configuration front, make sure its configurable via the wayback.xml. Allowing a -D override is good too.

@gmj2053
Copy link

gmj2053 commented Dec 8, 2014

Thanks Kris, if you need a use case why. I am moving towards one big index, as I get space, but I still need "collections" that are bounded by dates for which I use the wayback.xml prop.

@kris-sigur
Copy link
Member

Sounds like maybe the date limits should be defined per access point.

@kris-sigur
Copy link
Member

Thinking about the implications of changing this. It seems like an API change and we should move this to 2.1.0.

@gmj2053
Copy link

gmj2053 commented Dec 8, 2014

There are other elements we define using the access point, to include a change of template and auth by access point for off-site access, which I need to get back to testing. I have that set up in 1.6 but haven't yet figured it out in OW.

@RogerMathisen
Copy link
Contributor

I have now made changes to the Timestamp class so that the start date is configurable and the configuration can be reached from other parts of the code. So far I have also modified the .jsp pages refered in Erik Hetzners post so that they make use of the dynamically set start year. I also have made modifications that calculates the end (current) year on every request instead of on instance startup. This should take care of the problems that arises when an instance is up and running through new years. The modifications made might also have fixed the problems in issue #86, but we have not been able to verify this. I have pushed the new code to the nlnwa copy of Openwayback, and are planning to make a pull request for it into the iipc codebase.

The Timestamp modifications are as follows:

  • Made the YEAR_LOWER_LIMIT configurable.
  • Removed the YEAR_UPPER_LIMIT static final variable, and replaced usages of it with String.valueOf(Calendar.getInstance(TimeZone.getTimeZone("GMT")).get(Calendar.YEAR) )).
    This is because if the WayBack instance is up and running over between years. The new year will not be selectable.
  • Removed the static definition of the String array months, containing the shorthand name for the months in a year, as it was it was hardcoded to use the english abbreviations.
    The new version relies on the default Locale settings to supply the shorthand names.
  • Removed the array containing the number of days in each month in the defined year interval 1972 - 2032, and altered the getDaysInMonthBound() to use Calendar directly
    instead of relying on the pregenerated array.
  • Removed the SSE_1996 variable with SSE_YEAR_LOWER_LIMIT since the start year is no longer hardcoded. The value of the new variable is calculated at Class instansiation
    based on the configured start year.
  • Altered the dateStrToDate() function. The return value under ParseException must be multiplied by 1000 to convert from the internal second since Epoch to Date() milliseconds.
    Also we use SSE_YEAR_LOWER_LIMIT instead of SSE_1996.
  • Altered the earliestTimestamp() function to SSE_YEAR_LOWER_LIMIT instead of SSE_1996.
  • Exposted the value of start and end year through getter methods.
  • Added setter method for start year to make it configurable through wayback.xml

@RogerMathisen
Copy link
Contributor

I also tried to locate a relevant CHANGES.md (webarchive-commons has one) to enter the modifications, but could not find such file in OpenWayback Core. The modifications should perhaps not be added until a pull request is accepted?

@kris-sigur
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants