22189
Comment:
|
← Revision 16 as of 2010-08-16 10:24:44 ⇥
21827
converted to 1.6 markup
|
Deletions are marked like this. | Additions are marked like this. |
Line 10: | Line 10: |
{{{ | {{{ |
Line 14: | Line 14: |
{{{ | {{{ |
Line 19: | Line 20: |
Line 22: | Line 22: |
|| Java logging is used a couple of places in the Deploy code. And we're not running DeployApplication with a log.properties assigned to it. Consider using the same log.prop when running the DeployApplication, so the any DeployApplication logging is written to logs: -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger -Djava.util.logging.config.file=/home/test/SVC/conf/log_ArcRepositoryApplication.prop || Cosmetic || NOTOK || || Our use of the word arcrepository indicates the storage of arcfiles. Consider renaming "arcrepository" as "repository" || Cosmetic || NOTOK || |
|| Java logging is used a couple of places in the Deploy code. And we're not running DeployApplication with a log.properties assigned to it. Consider using the same log.prop when running the DeployApplication, so the any DeployApplication logging is written to logs: -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger -Djava.util.logging.config.file=/home/test/SVC/conf/log_ArcRepositoryApplication.prop || Cosmetic || POSTPONE || || Our use of the word arcrepository indicates the storage of arcfiles. Consider renaming "arcrepository" as "repository" || Cosmetic || POSTPONE || |
Line 30: | Line 28: |
|| 161 || make "_" as a constant || Cosmetic || NOTOK || || 180 || Spelling: Receving => Receiving || Cosmetic || NOTOK || || 300 || Spelling: Recieving => Receiving || Cosmetic || NOTOK || || 338 || Spelling: Receving => Receiving || Cosmetic || NOTOK || |
|| 161 || make "_" as a constant || Cosmetic || OK || || 180 || Spelling: Receving => Receiving || Cosmetic || OK || || 300 || Spelling: Recieving => Receiving || Cosmetic || OK || || 338 || Spelling: Receving => Receiving || Cosmetic || OK || |
Line 36: | Line 34: |
|| 72 || Add validation: fDate/cDate (not null), the String values (not null not empty), the int/long values (positive integers) || Cosmetic || NOTOK || || 164 || rename method: getFileListCheckdatetime() || Cosmetic || NOTOK || |
|| 72 || Add validation: fDate/cDate (not null), the String values (not null not empty), the int/long values (positive integers) || Cosmetic || OK || || 164 || rename method: getFileListCheckdatetime() || Cosmetic || OK || |
Line 40: | Line 38: |
|| 237 || That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions. In anycase the javadoc for BitarchiveReply should describe our chosen semantics || Cosmetic || NOTOK || || 312 || spelling: live => alive || Cosmetic || NOTOK || |
|| 237 || That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions. In anycase the javadoc for BitarchiveReply should describe our chosen semantics || Cosmetic || INVALID || || 312 || spelling: live => alive || Cosmetic || OK || |
Line 44: | Line 42: |
|| 125 || ID => identifiers || Cosmetic || NOTOK || || 348 || Add to logentry and to javadoc: Returning the first one || Cosmetic || NOTOK || || 378 || Add: Returning the first one || Cosmetic || NOTOK || || 877 || Implement TODO, or remove TODO || Cosmetic || NOTOK || || 1386 || Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED)) || Cosmetic || NOTOK || || 1788 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || NOTOK || || 1821 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || NOTOK || || 1989 || spelling: bitarche => bitarchive || Cosmetic || NOTOK || || 2061 || Missing validation of argument 'line' || Cosmetic || NOTOK || || 2079-2080 || Remove? || Cosmetic || NOTOK || || 2132 || Add null validation of 'date' argument || Cosmetic || NOTOK || || 2160 || Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment! || Cosmetic || NOTOK || |
|| 125 || ID => identifiers || Cosmetic || OK || || 348 || Add to logentry and to javadoc: Returning the first one || Cosmetic || OK || || 378 || Add: Returning the first one || Cosmetic || OK || || 877 || Implement TODO, or remove TODO || Cosmetic || OK || || 1386 || Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED)) || Cosmetic || OK || || 1788 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || OK || || 1821 || This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc || Cosmetic || OK || || 1989 || spelling: bitarche => bitarchive || Cosmetic || OK || || 2061 || Missing validation of argument 'line' || Cosmetic || OK || || 2079-2080 || Remove? || Cosmetic || OK || || 2132 || Add null validation of 'date' argument || Cosmetic || OK || || 2160 || Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment! || Cosmetic || OK || |
Line 60: | Line 58: |
|| 537 || Spelling: Recieved => Received || Cosmetic || NOTOK || || 597 || Spelling: Recieved => Received || Cosmetic || NOTOK || || 657 || Spelling: Recieved => Received || Cosmetic || NOTOK || |
|| 537 || Spelling: Recieved => Received || Cosmetic || OK || || 597 || Spelling: Recieved => Received || Cosmetic || OK || || 657 || Spelling: Recieved => Received || Cosmetic || OK || |
Line 65: | Line 63: |
|| 1316-1318 || Explain, that the output of shutdown is extended to the same file as the output when starting the database. || Cosmetic || NOTOK || | || 1316-1318 || Explain, that the output of shutdown is extended to the same file as the output when starting the database. || Cosmetic || OK || |
Line 68: | Line 66: |
|| General || missing SVN:keywords || Cosmetic || NOTOK || || 145 || Make into local constant or include with dk.netarkivet.archive.Constants || Cosmetic || NOTOK || || 165 || spelling: occured => occurred || Cosmetic || NOTOK || |
|| General || missing SVN:keywords || Cosmetic || OK || || 145 || Make into local constant or include with dk.netarkivet.archive.Constants || Cosmetic || OK || || 165 || spelling: occured => occurred || Cosmetic || OK || |
Line 73: | Line 71: |
|| 70 || Please rename to OPTION_SECURITY_POLICY || Cosmetic || NOTOK || || 72 || Please rename to OPTION_SECURITY_POLICY_WIN || Cosmetic || NOTOK || || 74 || This is not an appropriate description. || Cosmetic || NOTOK || || 81-82 || Change to one line || Cosmetic || NOTOK || || 84-85 || Change to one line || Cosmetic || NOTOK || || 118 || change to ETC_PROFILE || Cosmetic || NOTOK || || 125-128 || Replace with calls to StringUtils.repeat || Cosmetic || NOTOK || || 356, 369, 382, 398, 414 || These checkNotNulls should be replaced with checkNotNullOrEmpty || Cosmetic || NOTOK || || 429, 447-449 || These checkNotNulls should be replaced with checkNotNullOrEmpty || Cosmetic || NOTOK || |
|| 70 || Please rename to OPTION_SECURITY_POLICY || Cosmetic || OK || || 72 || Please rename to OPTION_SECURITY_POLICY_WIN || Cosmetic || OK || || 74 || This is not an appropriate description. || Cosmetic || OK || || 81-82 || Change to one line || Cosmetic || OK || || 84-85 || Change to one line || Cosmetic || OK || || 118 || change to ETC_PROFILE || Cosmetic || OK || || 125-128 || Replace with calls to StringUtils.repeat || Cosmetic || REJECT || || 356, 369, 382, 398, 414 || These checkNotNulls should be replaced with checkNotNullOrEmpty || Cosmetic || OK || || 429, 447-449 || These checkNotNulls should be replaced with checkNotNullOrEmpty || Cosmetic || OK || |
Line 84: | Line 82: |
|| 84 || replace 'to' with 'ChannelID to'. || Cosmetic || NOTOK || || 85-86 || replace 'baAppId' with 'String baAppId' || Cosmetic || NOTOK || || 88 || replace 'originatingBatchMsgId' with 'String originatingBatchMsgId' || Cosmetic || NOTOK || |
|| 84 || replace 'to' with 'ChannelID to'. || Cosmetic || OK || || 85-86 || replace 'baAppId' with 'String baAppId' || Cosmetic || OK || || 88 || replace 'originatingBatchMsgId' with 'String originatingBatchMsgId' || Cosmetic || OK || |
Line 89: | Line 87: |
|| General || There are multiple arguments in this class named "arcfileName". Rename as "fileName" || Cosmetic || NOTOK || || General || All references til "arcrepository" should maybe be just "repository" || Cosmetic || NOTOK || || 106 || spelling: "ddmin data" => "admin data" || Cosmetic || NOTOK || || 109, 191, 195 || This is used in AdminData.toString method to include there information about known bitarchives. The knownBitarchives could probably now be replaced by a database call || Cosmetic || NOTOK || |
|| General || There are multiple arguments in this class named "arcfileName". Rename as "fileName" || Cosmetic || OK || || General || All references til "arcrepository" should maybe be just "repository" || Cosmetic || POSTPONE || || 106 || spelling: "ddmin data" => "admin data" || Cosmetic || OK || || 109, 191, 195 || This is used in AdminData.toString method to include there information about known bitarchives. The knownBitarchives could probably now be replaced by a database call || Cosmetic || REJECT || |
Line 95: | Line 93: |
|| 173 || Removal initial \n || Cosmetic || NOTOK || | || 173 || Removal initial \n || Cosmetic || OK || |
Line 98: | Line 96: |
|| 94, 103 || rename "bpdbFileName" as "arcdatabaseFilename" || Cosmetic || NOTOK || || 681 || Rename JMXREMOTE_HERITRIX_PRIVELEGES as JMXREMOTE_HERITRIX_PRIVILEGES || Cosmetic || NOTOK || |
|| 94, 103 || rename "bpdbFileName" as "arcdatabaseFilename" || Cosmetic || OK || || 681 || Rename JMXREMOTE_HERITRIX_PRIVELEGES as JMXREMOTE_HERITRIX_PRIVILEGES || Cosmetic || OK || |
Line 102: | Line 100: |
|| 162 || Spelling: matchin=> matching || Cosmetic || NOTOK || | || 162 || Spelling: matchin=> matching || Cosmetic || OK || |
Line 105: | Line 103: |
|| 33 || Why has this been added, when it is not used? || Cosmetic || NOTOK || || 46 || rename 'e' as 'root' || Cosmetic || NOTOK || || 868-869 || This indentation looks quite odd. Insert line break. || Cosmetic || NOTOK || |
|| 33 || Why has this been added, when it is not used? || Cosmetic || OK || || 46 || rename 'e' as 'root' || Cosmetic || OK || || 868-869 || This indentation looks quite odd. Insert line break. || Cosmetic || OK || |
Line 110: | Line 108: |
|| 123 || Maybe, you should rather write: map (filename -> checksum) || Cosmetic || NOTOK || || 140 || Consider adding 'synchronized' to this method || Cosmetic || NOTOK || || 341-346 || Test whether the file is readable || Cosmetic || NOTOK || || 360 || make as a constant || Cosmetic || NOTOK || || 400 || Spelling: occured => occurred || Cosmetic || NOTOK || || 529-550, 553-574 || Duplicate code here; Have only one method and return a dk.netarkivet.common.utils.KeyValuePair instead || Cosmetic || NOTOK || || 605 || spelling: occured => occurred || Cosmetic || NOTOK || || 645-657 || Replace 'arcFile' with 'file'; this code accepts (and should accept) any kind of files || Cosmetic || NOTOK || |
|| 123 || Maybe, you should rather write: map (filename -> checksum) || Cosmetic || OK || || 140 || Consider adding 'synchronized' to this method || Cosmetic || OK || || 341-346 || Test whether the file is readable || Cosmetic || OK || || 360 || make as a constant || Cosmetic || OK || || 400 || Spelling: occured => occurred || Cosmetic || OK || || 529-550, 553-574 || Duplicate code here; Have only one method and return a dk.netarkivet.common.utils.KeyValuePair instead || Cosmetic || OK || || 605 || spelling: occured => occurred || Cosmetic || OK || || 645-657 || Replace 'arcFile' with 'file'; this code accepts (and should accept) any kind of files || Cosmetic || OK || |
Line 120: | Line 118: |
|| General || Add missing svn:keywords || Cosmetic || NOTOK || || 67 || Add "synchronized" to the method to prevent synchronization problems || Cosmetic || NOTOK || || 117 || add: uniquely identifying the replica || Cosmetic || NOTOK || || 141-142 || Is this still an outstanding issue? Add FR to handle this issue about storestate validity || Cosmetic || NOTOK || || 295-296 || Make a more meaningful text for this exception || Cosmetic || NOTOK || || 346-348 || Can be replaced with just instance = null; || Cosmetic || NOTOK || |
|| General || Add missing svn:keywords || Cosmetic || OK || || 67 || Add "synchronized" to the method to prevent synchronization problems || Cosmetic || OK || || 117 || add: uniquely identifying the replica || Cosmetic || OK || || 141-142 || Is this still an outstanding issue? Add FR to handle this issue about storestate validity || Cosmetic || REJECT || || 295-296 || Make a more meaningful text for this exception || Cosmetic || OK || || 346-348 || Can be replaced with just instance = null; || Cosmetic || OK || |
Line 128: | Line 126: |
|| 45-46 || Or instead of "Inheriting the ": Must implement the java.lang.Comparable interface || NA || NOTOK || | || 45-46 || Or instead of "Inheriting the ": Must implement the java.lang.Comparable interface || NA || OK || |
Line 131: | Line 129: |
|| General || add SVN:keywords || Cosmetic || NOTOK || | || General || add SVN:keywords || Cosmetic || OK || |
Line 134: | Line 132: |
|| 423 || Add @throws PermissionDenied if the credentials for correcting files are wrong || Cosmetic || NOTOK || | || 423 || Add @throws PermissionDenied if the credentials for correcting files are wrong || Cosmetic || OK || |
Line 137: | Line 135: |
|| 221 || Clarify your TODO. This is also about if or when the checksum information from the replica expires, isn't it? || Cosmetic || NOTOK || || 436 || Add: valid meaning not null and not empty || Cosmetic || NOTOK || || 463 || add: invalid meaning not null; Should we accept not empty filenames here? || Cosmetic || NOTOK || |
|| 221 || Clarify your TODO. This is also about if or when the checksum information from the replica expires, isn't it? || Cosmetic ||OK || || 436 || Add: valid meaning not null and not empty || Cosmetic || OK || || 463 || add: invalid meaning not null; Should we accept not empty filenames here? || Cosmetic || OK || |
Line 142: | Line 140: |
|| 45, 61, 67, 70 || Rename as "filename" || Cosmetic || NOTOK || || 138-140 || Rename as getFilename() || Cosmetic || NOTOK || || 182 || Arcfile => File || Cosmetic || NOTOK || |
|| 45, 61, 67, 70 || Rename as "filename" || Cosmetic || OK || || 138-140 || Rename as getFilename() || Cosmetic || OK || || 182 || Arcfile => File || Cosmetic || OK || |
Line 147: | Line 145: |
|| 254 || Add "." after "below here" || Cosmetic || NOTOK || | || 254 || Add "." after "below here" || Cosmetic || OK || |
Line 150: | Line 148: |
|| 95 || Wrong comment: A new instance is now created every time || Cosmetic || NOTOK || | || 95 || Wrong comment: A new instance is now created every time || Cosmetic || OK || |
Line 153: | Line 151: |
|| 139 || Change constant COMPLETE_HARVEST_HETRIX_GUI_PORT to COMPLETE_HARVEST_HERITRIX_GUI_PORT || Cosmetic || NOTOK || || 141 || Change constant COMPLETE_HARVEST_HETRIX_JMX_PORT to COMPLETE_HARVEST_HERITRIX_JMX_PORT || Cosmetic || NOTOK || |
|| 139 || Change constant COMPLETE_HARVEST_HETRIX_GUI_PORT to COMPLETE_HARVEST_HERITRIX_GUI_PORT || Cosmetic || OK || || 141 || Change constant COMPLETE_HARVEST_HETRIX_JMX_PORT to COMPLETE_HARVEST_HERITRIX_JMX_PORT || Cosmetic || OK || |
Line 157: | Line 155: |
|| 199 || Remove line || Cosmetic || NOTOK || | || 199 || Remove line || Cosmetic || OK || |
Line 160: | Line 158: |
|| 246-249 || Consider removing this comment, as this is not a priority to HelpFormatter instead. And furthermore I don't understand the problem statement "HelpFormater only prints directly". It doesn't seem to be true. If it is a problem with cli-1.0 then it could be solved in the latest cli-release (1.2; released march 19, 2009) || NA || NOTOK || | || 246-249 || Consider removing this comment, as this is not a priority to HelpFormatter instead. And furthermore I don't understand the problem statement "HelpFormater only prints directly". It doesn't seem to be true. If it is a problem with cli-1.0 then it could be solved in the latest cli-release (1.2; released march 19, 2009) || NA || OK || |
Line 163: | Line 161: |
|| General || The mention of "bitarchives" should be replaced with replica. This is both an issue regarding javadoc, method-naming, and argument naming (i.e. Replica bitarchive in method getBitarchiveChecksum()) || Cosmetic || NOTOK || || 73 || We're not validating the checksumMap, here. Is that on purpose? Validate for null and not empty map || Cosmetic || NOTOK || || 107 || The localisation should probably take place in the ReplicaStoreState class. This is possible if the getAdminBitarchiveStoreState does not return null in the case of "No state", but instead returns a ReplicaStoreState object representing "No state" || Cosmetic || NOTOK || |
|| General || The mention of "bitarchives" should be replaced with replica. This is both an issue regarding javadoc, method-naming, and argument naming (i.e. Replica bitarchive in method getBitarchiveChecksum()) || Cosmetic || OK || || 73 || We're not validating the checksumMap, here. Is that on purpose? Validate for null and not empty map || Cosmetic || OK || || 107 || The localisation should probably take place in the ReplicaStoreState class. This is possible if the getAdminBitarchiveStoreState does not return null in the case of "No state", but instead returns a ReplicaStoreState object representing "No state" || Cosmetic || OK || |
Line 168: | Line 166: |
|| 51 || Spelling: The type of cache || Cosmetic || NOTOK || | || 51 || Spelling: The type of cache || Cosmetic || OK || |
Line 173: | Line 171: |
|| 86 || Consider adding a log.warn message at this point || Cosmetic || NOTOK || | || 86 || Consider adding a log.warn message at this point || Cosmetic || OK || |
Line 176: | Line 174: |
|| 32 || Add to javadoc, why this class is so short! || Cosmetic || NOTOK || | || 32 || Add to javadoc, why this class is so short! || Cosmetic || OK || |
Line 179: | Line 177: |
|| 30 || => This is used by .. || Cosmetic || NOTOK || || 43-44 || certain => specific || Cosmetic || NOTOK || |
|| 30 || => This is used by .. || Cosmetic || OK || || 43-44 || certain => specific || Cosmetic || OK || |
Line 183: | Line 181: |
|| General || Consider adding validation for the visit() methods in this class || Cosmetic || NOTOK || | || General || Consider adding validation for the visit() methods in this class || Cosmetic || OK || |
Line 186: | Line 184: |
|| 962 || Remove TODO; as we don't intend to implement this method || Cosmetic || NOTOK || || 975 || Remove TODO; as we don't intend to implement this method || Cosmetic || NOTOK || |
|| 962 || Remove TODO; as we don't intend to implement this method || Cosmetic || OK || || 975 || Remove TODO; as we don't intend to implement this method || Cosmetic || OK || |
Line 190: | Line 188: |
|| 168, 171, 181 || HETRIX => HERITRIX || Cosmetic || NOTOK || || 325 || Rename file? bpdb.jar refers to a bitpreservation database file, and the database is now much more. || Cosmetic || NOTOK || || 518 || Archive database ... || Minor || NOTOK || || 577, 590 || Be more strict: Disallow empty strings as valid scopes || Cosmetic || NOTOK || |
|| 168, 171, 181 || HETRIX => HERITRIX || Cosmetic || OK || || 325 || Rename file? bpdb.jar refers to a bitpreservation database file, and the database is now much more. || Cosmetic || OK || || 518 || Archive database ... || Minor || OK || || 577, 590 || Be more strict: Disallow empty strings as valid scopes || Cosmetic || OK || |
Line 196: | Line 194: |
|| 241 || Is this still true? If not, remove line || Cosmetic || NOTOK || || 255 || Change to "Failed to handle GetAllFilenamesMessage: " ... || Cosmetic || NOTOK || || 276 || Change to "Failed to handle GetAllChecksumsMessage: " ... || Cosmetic || NOTOK || || 292 || Spelling: Recieved => Received || Cosmetic || NOTOK || || 319 || Is this still true? If not remove TODO line || Cosmetic || NOTOK || |
|| 241 || Is this still true? If not, remove line || Cosmetic || OK || || 255 || Change to "Failed to handle GetAllFilenamesMessage: " ... || Cosmetic || OK || || 276 || Change to "Failed to handle GetAllChecksumsMessage: " ... || Cosmetic || OK || || 292 || Spelling: Recieved => Received || Cosmetic || OK || || 319 || Is this still true? If not remove TODO line || Cosmetic || OK || |
Line 203: | Line 201: |
|| 180 || Add: Currently does nothing || Cosmetic || NOTOK || || 185-191, 205 || throws IOFailure || Cosmetic || NOTOK || || 206-210 || delete? || Cosmetic || NOTOK || || 216 || Add: Currently does nothing || Cosmetic || NOTOK || |
|| 180 || Add: Currently does nothing || Cosmetic || OK || || 185-191, 205 || throws IOFailure || Cosmetic || OK || || 206-210 || delete? || Cosmetic || OK || || 216 || Add: Currently does nothing || Cosmetic || OK || |
Line 209: | Line 207: |
|| 44-49 || Javadoc is misleading; Change to: Method for shutting down the database system. Has no effect in this implementation Maybe log a warning about this fact when the method is called. || Cosmetic || NOTOK || | || 44-49 || Javadoc is misleading; Change to: Method for shutting down the database system. Has no effect in this implementation Maybe log a warning about this fact when the method is called. || Cosmetic || OK || |
Line 212: | Line 210: |
|| General || missing license header || Cosmetic || NOTOK || || General || Replace all(?) instances of "bitarchive" with "replica" in this class || Cosmetic || NOTOK || || General || Consider adding a featurerequest for some of TODOs || Cosmetic || NOTOK || || General || Consider replacing == with equals, if not certain that they have equal semantics || Cosmetic || NOTOK || || 42 || rename "file" as "filename" || Cosmetic || NOTOK || || 92-93 || Is this correct behaviour and if yes, document it in the javadoc. || Cosmetic || NOTOK || || 98 || If this is for presentation purposes only, move it to the archive.webinterface package || Cosmetic || NOTOK || || 134-136 || Add to javadoc, that it always returns true || Cosmetic || NOTOK || || 158 || Complete logmessage || Cosmetic || NOTOK || |
|| General || missing license header || Cosmetic || OK || || General || Replace all(?) instances of "bitarchive" with "replica" in this class || Cosmetic || OK || || General || Consider adding a featurerequest for some of TODOs || Cosmetic || OK || || General || Consider replacing == with equals, if not certain that they have equal semantics || Cosmetic || OK || || 42 || rename "file" as "filename" || Cosmetic || OK || || 92-93 || Is this correct behaviour and if yes, document it in the javadoc. || Cosmetic || OK || || 98 || If this is for presentation purposes only, move it to the archive.webinterface package || Cosmetic || OK || || 134-136 || Add to javadoc, that it always returns true || Cosmetic || OK || || 158 || Complete logmessage || Cosmetic || OK || |
Line 223: | Line 221: |
|| 211-217 || why make a variable which is only used once? Make the string in the creating of the exception. || Cosmetic || NOTOK || || 396 || Refactor to be a method inside one of the Admin implementations || Cosmetic || NOTOK || |
|| 211-217 || why make a variable which is only used once? Make the string in the creating of the exception. || Cosmetic || OK || || 396 || Refactor to be a method inside one of the Admin implementations || Cosmetic || POSTPONE || |
Line 227: | Line 225: |
|| 114 || Will this only work for an external database? If no update javadoc. || Cosmetic || NOTOK || | || 114 || Will this only work for an external database? If no update javadoc. || Cosmetic || OK || |
Line 230: | Line 228: |
|| 126-129 || Too long lines || Cosmetic || NOTOK || | || 126-129 || Too long lines || Cosmetic || OK || |
Line 233: | Line 231: |
|| General || Add svn:keywords || Cosmetic || NOTOK || || 45 || add newline || Cosmetic || NOTOK || || 61 || add newline || Cosmetic || NOTOK || || 64-65 || Deprecate this method; as in the future it will not be possible to setChecksum || Cosmetic || NOTOK || || 79 || add newline || Cosmetic || NOTOK || || 86 || add newline || Cosmetic || NOTOK || || 104 || add newline || Cosmetic || NOTOK || || 108 || This is not decided in the interface, but in the actual implementation(s) || Cosmetic || NOTOK || || 116 || add newline || Cosmetic || NOTOK || |
|| General || Add svn:keywords || Cosmetic || OK || || 45 || add newline || Cosmetic || OK || || 61 || add newline || Cosmetic || OK || || 64-65 || Deprecate this method; as in the future it will not be possible to setChecksum || Cosmetic || OK || || 79 || add newline || Cosmetic || OK || || 86 || add newline || Cosmetic || OK || || 104 || add newline || Cosmetic || OK || || 108 || This is not decided in the interface, but in the actual implementation(s) || Cosmetic || OK || || 116 || add newline || Cosmetic || OK || |
Line 244: | Line 242: |
|| 74 || throws ArgumentNotValid! || Cosmetic || NOTOK || || 90 || throws ArgumentNotValid || Cosmetic || NOTOK || |
|| 74 || throws ArgumentNotValid! || Cosmetic || OK || || 90 || throws ArgumentNotValid || Cosmetic || OK || |
Line 248: | Line 246: |
|| General || Add svn:keywords || Cosmetic || NOTOK || | || General || Add svn:keywords || Cosmetic || OK || |
Line 251: | Line 249: |
|| General || Consider adding validation for the visit methods in this class || Cosmetic || NOTOK || | || General || Consider adding validation for the visit methods in this class || Cosmetic || OK || |
Review (NS-147): Archive Assignment B.2.2 complete
Author |
Jonas |
Moderator |
Jonas |
State |
Closed |
Objectives
Besides containing all the latest work on database archival, it also contains a lot of code style changes.
Summary
Follow up by JOLF
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) JOLF: 8 MD SVC: 2 MD
General comments:
Description |
Classification |
Status |
Java logging is used a couple of places in the Deploy code. And we're not running DeployApplication with a log.properties assigned to it. Consider using the same log.prop when running the DeployApplication, so the any DeployApplication logging is written to logs: -Dorg.apache.commons.logging.Log=org.apache.commons.logging.impl.Jdk14Logger -Djava.util.logging.config.file=/home/test/SVC/conf/log_ArcRepositoryApplication.prop |
Cosmetic |
POSTPONE |
Our use of the word arcrepository indicates the storage of arcfiles. Consider renaming "arcrepository" as "repository" |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1320
Lines |
Description |
Classification |
Status |
161 |
make "_" as a constant |
Cosmetic |
OK |
180 |
Spelling: Receving => Receiving |
Cosmetic |
OK |
300 |
Spelling: Recieving => Receiving |
Cosmetic |
OK |
338 |
Spelling: Receving => Receiving |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaFileInfo.java', revision 1283
Lines |
Description |
Classification |
Status |
72 |
Add validation: fDate/cDate (not null), the String values (not null not empty), the int/long values (positive integers) |
Cosmetic |
OK |
164 |
rename method: getFileListCheckdatetime() |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/BitarchiveMonitor.java', revision 1283
Lines |
Description |
Classification |
Status |
237 |
That depends on the semantics, we want to impose: If exceptions is null means no exceptions, null is ok; otherwise we should disallow null; and enforce an empty list meaning no exceptions. In anycase the javadoc for BitarchiveReply should describe our chosen semantics |
Cosmetic |
INVALID |
312 |
spelling: live => alive |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ReplicaCacheDatabase.java', revision 1311
Lines |
Description |
Classification |
Status |
125 |
ID => identifiers |
Cosmetic |
OK |
348 |
Add to logentry and to javadoc: Returning the first one |
Cosmetic |
OK |
378 |
Add: Returning the first one |
Cosmetic |
OK |
877 |
Implement TODO, or remove TODO |
Cosmetic |
OK |
1386 |
Dangerous use of ==. May or not work as intended Use instead if (us.equals(ReplicaStoreState.UPLOAD_COMPLETED)) |
Cosmetic |
OK |
1788 |
This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc |
Cosmetic |
OK |
1821 |
This can tricker IllegalArgumentException if parsing of the result fails. Add this information to javadoc |
Cosmetic |
OK |
1989 |
spelling: bitarche => bitarchive |
Cosmetic |
OK |
2061 |
Missing validation of argument 'line' |
Cosmetic |
OK |
2079-2080 |
Remove? |
Cosmetic |
OK |
2132 |
Add null validation of 'date' argument |
Cosmetic |
OK |
2160 |
Can we kill this method now, or should we wait to the next stable release? No, just kill FIXME comment! |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/settings.xml', revision 1300
Lines |
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1295
Lines |
Description |
Classification |
Status |
537 |
Spelling: Recieved => Received |
Cosmetic |
OK |
597 |
Spelling: Recieved => Received |
Cosmetic |
OK |
657 |
Spelling: Recieved => Received |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/deploy/LinuxMachine.java', revision 1305
Lines |
Description |
Classification |
Status |
1316-1318 |
Explain, that the output of shutdown is extended to the same file as the output when starting the database. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/tools/ReestablishAdminDatabase.java', revision 1311
Lines |
Description |
Classification |
Status |
General |
missing SVN:keywords |
Cosmetic |
OK |
145 |
Make into local constant or include with dk.netarkivet.archive.Constants |
Cosmetic |
OK |
165 |
spelling: occured => occurred |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/deploy/ScriptConstants.java', revision 1305
Lines |
Description |
Classification |
Status |
70 |
Please rename to OPTION_SECURITY_POLICY |
Cosmetic |
OK |
72 |
Please rename to OPTION_SECURITY_POLICY_WIN |
Cosmetic |
OK |
74 |
This is not an appropriate description. |
Cosmetic |
OK |
81-82 |
Change to one line |
Cosmetic |
OK |
84-85 |
Change to one line |
Cosmetic |
OK |
118 |
change to ETC_PROFILE |
Cosmetic |
OK |
125-128 |
Replace with calls to StringUtils.repeat |
Cosmetic |
REJECT |
356, 369, 382, 398, 414 |
These checkNotNulls should be replaced with checkNotNullOrEmpty |
Cosmetic |
OK |
429, 447-449 |
These checkNotNulls should be replaced with checkNotNullOrEmpty |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BatchEndedMessage.java', revision 1289
Lines |
Description |
Classification |
Status |
84 |
replace 'to' with 'ChannelID to'. |
Cosmetic |
OK |
85-86 |
replace 'baAppId' with 'String baAppId' |
Cosmetic |
OK |
88 |
replace 'originatingBatchMsgId' with 'String originatingBatchMsgId' |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/UpdateableAdminData.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
There are multiple arguments in this class named "arcfileName". Rename as "fileName" |
Cosmetic |
OK |
General |
All references til "arcrepository" should maybe be just "repository" |
Cosmetic |
POSTPONE |
106 |
spelling: "ddmin data" => "admin data" |
Cosmetic |
OK |
109, 191, 195 |
This is used in AdminData.toString method to include there information about known bitarchives. The knownBitarchives could probably now be replaced by a database call |
Cosmetic |
REJECT |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 1298
Lines |
Description |
Classification |
Status |
173 |
Removal initial \n |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/deploy/Machine.java', revision 1302
Lines |
Description |
Classification |
Status |
94, 103 |
rename "bpdbFileName" as "arcdatabaseFilename" |
Cosmetic |
OK |
681 |
Rename JMXREMOTE_HERITRIX_PRIVELEGES as JMXREMOTE_HERITRIX_PRIVILEGES |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CDXOriginCrawlLogIterator.java', revision 1289
Lines |
Description |
Classification |
Status |
162 |
Spelling: matchin=> matching |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/deploy/WindowsMachine.java', revision 1302
Lines |
Description |
Classification |
Status |
33 |
Why has this been added, when it is not used? |
Cosmetic |
OK |
46 |
rename 'e' as 'root' |
Cosmetic |
OK |
868-869 |
This indentation looks quite odd. Insert line break. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1320
Lines |
Description |
Classification |
Status |
123 |
Maybe, you should rather write: map (filename -> checksum) |
Cosmetic |
OK |
140 |
Consider adding 'synchronized' to this method |
Cosmetic |
OK |
341-346 |
Test whether the file is readable |
Cosmetic |
OK |
360 |
make as a constant |
Cosmetic |
OK |
400 |
Spelling: occured => occurred |
Cosmetic |
OK |
529-550, 553-574 |
Duplicate code here; Have only one method and return a dk.netarkivet.common.utils.KeyValuePair instead |
Cosmetic |
OK |
605 |
spelling: occured => occurred |
Cosmetic |
OK |
645-657 |
Replace 'arcFile' with 'file'; this code accepts (and should accept) any kind of files |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DatabaseAdmin.java', revision 1285
Lines |
Description |
Classification |
Status |
General |
Add missing svn:keywords |
Cosmetic |
OK |
67 |
Add "synchronized" to the method to prevent synchronization problems |
Cosmetic |
OK |
117 |
add: uniquely identifying the replica |
Cosmetic |
OK |
141-142 |
Is this still an outstanding issue? Add FR to handle this issue about storestate validity |
Cosmetic |
REJECT |
295-296 |
Make a more meaningful text for this exception |
Cosmetic |
OK |
346-348 |
Can be replaced with just instance = null; |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/CombiningMultiFileBasedCache.java', revision 1283
Lines |
Description |
Classification |
Status |
45-46 |
Or instead of "Inheriting the ": Must implement the java.lang.Comparable interface |
NA |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/AdminFactory.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
add SVN:keywords |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 1321
Lines |
Description |
Classification |
Status |
423 |
Add @throws PermissionDenied if the credentials for correcting files are wrong |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabaseBasedActiveBitPreservation.java', revision 1293
Lines |
Description |
Classification |
Status |
221 |
Clarify your TODO. This is also about if or when the checksum information from the replica expires, isn't it? |
Cosmetic |
OK |
436 |
Add: valid meaning not null and not empty |
Cosmetic |
OK |
463 |
add: invalid meaning not null; Should we accept not empty filenames here? |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/RemoveAndGetFileMessage.java', revision 1283
Lines |
Description |
Classification |
Status |
45, 61, 67, 70 |
Rename as "filename" |
Cosmetic |
OK |
138-140 |
Rename as getFilename() |
Cosmetic |
OK |
182 |
Arcfile => File |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/WorkFiles.java', revision 1288
Lines |
Description |
Classification |
Status |
254 |
Add "." after "below here" |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1298
Lines |
Description |
Classification |
Status |
95 |
Wrong comment: A new instance is now created every time |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/deploy/CreateTestInstance.java', revision 1301
Lines |
Description |
Classification |
Status |
139 |
Change constant COMPLETE_HARVEST_HETRIX_GUI_PORT to COMPLETE_HARVEST_HERITRIX_GUI_PORT |
Cosmetic |
OK |
141 |
Change constant COMPLETE_HARVEST_HETRIX_JMX_PORT to COMPLETE_HARVEST_HERITRIX_JMX_PORT |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/distribute/IndexRequestServer.java', revision 1296
Lines |
Description |
Classification |
Status |
199 |
Remove line |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/tools/RunBatch.java', revision 1289
Lines |
Description |
Classification |
Status |
246-249 |
Consider removing this comment, as this is not a priority to HelpFormatter instead. And furthermore I don't understand the problem statement "HelpFormater only prints directly". It doesn't seem to be true. If it is a problem with cli-1.0 then it could be solved in the latest cli-release (1.2; released march 19, 2009) |
NA |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1294
Lines |
Description |
Classification |
Status |
General |
The mention of "bitarchives" should be replaced with replica. This is both an issue regarding javadoc, method-naming, and argument naming (i.e. Replica bitarchive in method getBitarchiveChecksum()) |
Cosmetic |
OK |
73 |
We're not validating the checksumMap, here. Is that on purpose? Validate for null and not empty map |
Cosmetic |
OK |
107 |
The localisation should probably take place in the ReplicaStoreState class. This is possible if the getAdminBitarchiveStoreState does not return null in the case of "No state", but instead returns a ReplicaStoreState object representing "No state" |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/FileBasedCache.java', revision 1289
Lines |
Description |
Classification |
Status |
51 |
Spelling: The type of cache |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileListJob.java', revision 1289
Lines |
Description |
Classification |
Status |
Comments on file 'trunk/src/dk/netarkivet/archive/webinterface/BitpreserveFileState.java', revision 1290
Lines |
Description |
Classification |
Status |
86 |
Consider adding a log.warn message at this point |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbySpecifics.java', revision 1285
Lines |
Description |
Classification |
Status |
32 |
Add to javadoc, why this class is so short! |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/FileListStatus.java', revision 1276
Lines |
Description |
Classification |
Status |
30 |
=> This is used by .. |
Cosmetic |
OK |
43-44 |
certain => specific |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveMonitorServer.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Consider adding validation for the visit() methods in this class |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1283
Lines |
Description |
Classification |
Status |
962 |
Remove TODO; as we don't intend to implement this method |
Cosmetic |
OK |
975 |
Remove TODO; as we don't intend to implement this method |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/deploy/Constants.java', revision 1304
Lines |
Description |
Classification |
Status |
168, 171, 181 |
HETRIX => HERITRIX |
Cosmetic |
OK |
325 |
Rename file? bpdb.jar refers to a bitpreservation database file, and the database is now much more. |
Cosmetic |
OK |
518 |
Archive database ... |
Minor |
OK |
577, 590 |
Be more strict: Disallow empty strings as valid scopes |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 1298
Lines |
Description |
Classification |
Status |
241 |
Is this still true? If not, remove line |
Cosmetic |
OK |
255 |
Change to "Failed to handle GetAllFilenamesMessage: " ... |
Cosmetic |
OK |
276 |
Change to "Failed to handle GetAllChecksumsMessage: " ... |
Cosmetic |
OK |
292 |
Spelling: Recieved => Received |
Cosmetic |
OK |
319 |
Is this still true? If not remove TODO line |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/indexserver/RawMetadataCache.java', revision 1310
Lines |
Description |
Classification |
Status |
180 |
Add: Currently does nothing |
Cosmetic |
OK |
185-191, 205 |
throws IOFailure |
Cosmetic |
OK |
206-210 |
delete? |
Cosmetic |
OK |
216 |
Add: Currently does nothing |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DerbyServerSpecifics.java', revision 1283
Lines |
Description |
Classification |
Status |
44-49 |
Javadoc is misleading; Change to: Method for shutting down the database system. Has no effect in this implementation Maybe log a warning about this fact when the method is called. |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/DatabasePreservationState.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
missing license header |
Cosmetic |
OK |
General |
Replace all(?) instances of "bitarchive" with "replica" in this class |
Cosmetic |
OK |
General |
Consider adding a featurerequest for some of TODOs |
Cosmetic |
OK |
General |
Consider replacing == with equals, if not certain that they have equal semantics |
Cosmetic |
OK |
42 |
rename "file" as "filename" |
Cosmetic |
OK |
92-93 |
Is this correct behaviour and if yes, document it in the javadoc. |
Cosmetic |
OK |
98 |
If this is for presentation purposes only, move it to the archive.webinterface package |
Cosmetic |
OK |
134-136 |
Add to javadoc, that it always returns true |
Cosmetic |
OK |
158 |
Complete logmessage |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1298
Lines |
Description |
Classification |
Status |
211-217 |
why make a variable which is only used once? Make the string in the creating of the exception. |
Cosmetic |
OK |
396 |
Refactor to be a method inside one of the Admin implementations |
Cosmetic |
POSTPONE |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/DBConnect.java', revision 1300
Lines |
Description |
Classification |
Status |
114 |
Will this only work for an external database? If no update javadoc. |
Cosmetic |
OK |
Comments on file 'trunk/webpages/BitPreservation/Bitpreservation-filestatus-checksum.jsp', revision 1288
Lines |
Description |
Classification |
Status |
126-129 |
Too long lines |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/Admin.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Add svn:keywords |
Cosmetic |
OK |
45 |
add newline |
Cosmetic |
OK |
61 |
add newline |
Cosmetic |
OK |
64-65 |
Deprecate this method; as in the future it will not be possible to setChecksum |
Cosmetic |
OK |
79 |
add newline |
Cosmetic |
OK |
86 |
add newline |
Cosmetic |
OK |
104 |
add newline |
Cosmetic |
OK |
108 |
This is not decided in the interface, but in the actual implementation(s) |
Cosmetic |
OK |
116 |
add newline |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepositoryadmin/ArchiveStoreState.java', revision 1283
Lines |
Description |
Classification |
Status |
74 |
throws ArgumentNotValid! |
Cosmetic |
OK |
90 |
throws ArgumentNotValid |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/PreservationState.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Add svn:keywords |
Cosmetic |
OK |
Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveServer.java', revision 1283
Lines |
Description |
Classification |
Status |
General |
Consider adding validation for the visit methods in this class |
Cosmetic |
OK |