Review (NS-125): Assignment B.2.2a: Checksum replica with FileBasedActiveBitPreservation

Author

Jonas

Moderator

Jonas

State

Closed

Objectives

The Checksum Replica are finished and it can now be handled by FileBasedActiveBitPreservation.
Also, check whether the unit test concerning the checksum replicas works properly (using test settings).

Total Time Used (Coding,Documentation,Review):

Time use (Coding,Documentation,Review)
HBK: 0.6
JOLF: 6

General comments:

Description

Classification

Status

Change ArgumentNotValid javadoc to contain the argument type along with the name

Cosmetic

OK

Add ' throws ArgumentNotValid' after metods throwing this execption. Check that it is in the code documentation.

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumFileServer.java', revision 1153

Lines

Description

Classification

Status

211-212

It does not remove the bad entry. It is moved from the archive file to the 'wrong' entry file instead.

Cosmetic

OK

222, 226

Why is it first checked whether an entry for the filename exists after it has been retrieved? The "if(!cs.hasEntry(filename))" could be replaced by "if(currentCs == null)", since currentCs is null if it was not found within the archive.

Minor

OK

230

comment that this exception is caught later in this function and used to give a NotOk reply the correct message. Do not log here!

Cosmetic

OK

236

String credentialsReceived

Cosmetic

OK

243-244

Do not return and do not set the message to NotOk here. Instead throw an exception, which can be caught later and used properly. Do not log here.

Cosmetic

OK

257-258

Is this file removed after use? It should.

Minor

OK

281

throws ArgumentNotValid in method header, now that its in java doc.

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FilePreservationState.java', revision 1149

Lines

Description

Classification

Status

213-216

Comment about this! Checks whether the replica has the correct checksum and that the replica is a bitarchive (e.g. that it actually has the file and not just the checksum).

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/JMSArcRepositoryClient.java', revision 1152

Lines

Description

Classification

Status

405-406

This is completely wrong. This should not be the replica Id, but the replica identification channel name.

Minor

INVALID

416

Insert type in ArguementNotValid

Cosmetic

OK

527

throws after method and javadoc

Cosmetic

OK

634-646

javadoc which exceptions are thrown. ArgumentNotValid and IOFailure.

Cosmetic

OK

640

Not name. The id of the replica.

Cosmetic

OK

648

throws after method and javadoc

Cosmetic

OK

665

remove comment

Cosmetic

OK

669

This could be misunderstood. Not convert to correct message but to the expected message type.

Cosmetic

OK

680-681

log the error message? Handle the error!

Minor

OK

705

throws after method and javadoc

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/FileBasedActiveBitPreservation.java', revision 1152

Lines

Description

Classification

Status

254

The list of filenames can potentially be more than 1,200,000 (The last figure I heard of the amount of files within Netarkivet), which fill the log files very fast (e.g. more than 20 MB per replica). Also, it is inefficient to create the final string for each replica. Restrict to 10 entries in the filenames list.

Minor

OK

274-275, 278, 282

The variable name checksumsForFileOnBa is refering only to BitArchives. Change to checksumForFileOnRep.

Cosmetic

OK

295

Do this!

Minor

OK

320

This TODO has been done. It now collects the checksums the indended way.

Cosmetic

OK

334

through the/a checksum

Cosmetic

OK

470

throws after method and javadoc

Cosmetic

OK

472

The WorkFiles.MISSING_FILES_BA should be changed to WorkFiles.MISSING_FILES_REPLICA.

Cosmetic

REJECT

538

This variable should be renamed to "extraFilesInReplica", since it is not BitArchive specific any more.

Cosmetic

OK

652-654

This does not need 3 lines.

Cosmetic

OK

717-720

Write what is done for the checksum replica.

Minor

OK

729

Not needed in private method

Cosmetic

OK

735-737

Comment about what is done here.

Cosmetic

OK

906

missing '.'

Cosmetic

OK

910-912

This does not need 3 lines.

Cosmetic

OK

921

bad comment. Remove bitarchive.

Cosmetic

OK

967

missing '.'

Cosmetic

OK

972

fewer lines

Cosmetic

OK

1026

throws after method and javadoc

Cosmetic

OK

1041-1042

combine these lines.

Cosmetic

OK

1061

The credentials for correction the bad entry. Like the other places.

Cosmetic

OK

1074

The variable name arcrep suggests that it is the ArcRepository and not a ArcRepositoryClient.

Cosmetic

OK

1157

throws after method and javadoc

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/distribute/ReplicaClient.java', revision 1152

Lines

Description

Classification

Status

General

public in front of interface method is give. Can be removed.

Cosmetic

INVALID

Comments on file 'trunk/src/dk/netarkivet/archive/bitarchive/distribute/BitarchiveClient.java', revision 1152

Lines

Description

Classification

Status

216-229

This method can only be called by the ArcRepository (or ArcRepositoryServer), and the CorrectMessages should only be sent by the BitPreservation. The ArcRepository should then resend the CorrectMessage to this BitarchiveClient, which is done in the following method. Thus this method should not exist.

Minor

OK

221

missing '.'

Cosmetic

INVALID

238, 251, 266, 281, 298-299

Make ArgumentNotValid checks.

Minor

OK

239-241

Handle correct messages !

Minor

OK

253-255

Handle GetAllFilenamesMessage!

Minor

OK

268-270

Handle GetAllChecksumsMessage!

Minor

OK

283-285

Handle GetChecksumMessage!

Minor

OK

300-302

Be able to create and send GetChecksumMessage based on the channel and filename.

Minor

OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/distribute/ArcRepositoryServer.java', revision 1152

Lines

Description

Classification

Status

239-252

Should this function not also cover Bitarchives ? Make Bug!

Minor

OK

258-271

As above. Checksum only. Make Bug!

Minor

OK

307-313

Write that this currently only works for ChecksumArchive. Also, make it work for bitarchives. Make Bug that it does not yet work for bitarchive replicas

Minor

OK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetChecksumMessage.java', revision 1148

Lines

Description

Classification

Status

62

Isn't second argument only parameter name?

Cosmetic

REJECT

123

Only name of variable

Cosmetic

REJECT

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/CorrectMessage.java', revision 1152

Lines

Description

Classification

Status

69

throws after method and javadoc

Cosmetic

OK

96

throws after method and javadoc Type on arguement

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/FileChecksumArchive.java', revision 1153

Lines

Description

Classification

Status

236

maybe change to, "create file if checksumFile do not exists."

Cosmetic

OK

271

comment on synchronized like below.

Cosmetic

OK

316

missing '.'

Cosmetic

REJECT

466, 469

We generally do not use IOException. Instead catch and throw new IOFailure.

Minor

OK

491

It does not use the double hash for separating the data and the wrong record. It uses colon surrounded by spaces instead.

Cosmetic

OK

520-521

Implement TODO!

Minor

REJECT

550

Yes, that would properly be best.

Minor

OK

663

Remove TODO and return sounds right.

Cosmetic

OK

664

rewrite!

Cosmetic

OK

691

Perhaps the checksumFile should be synchronized with the archive in memory?

Minor

OK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/GetAllFilenamesMessage.java', revision 1151

Lines

Description

Classification

Status

109

javadoc

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/common/distribute/arcrepository/LocalArcRepositoryClient.java', revision 1145

Lines

Description

Classification

Status

361-364

javadoc! Also, Implement this function. Check whether this is called! and handle this.

Minor

OK

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/ArcRepository.java', revision 1151

Lines

Description

Classification

Status

554

valid xhtml <br/>?

Cosmetic

OK

Comments on file 'trunk/src/dk/netarkivet/archive/checksum/distribute/ChecksumClient.java', revision 1152

Lines

Description

Classification

Status

94

throws after method and javadoc

Cosmetic

OK

104-132

This function is invalid and should be removed. The correct message should only be created by the bitpreservation and resend which is the case in the following method.

Minor

OK

134

javadoc

Cosmetic

OK

135

throws after method and javadoc

Cosmetic

OK

141, 157, 172, 189, 245

The resending log messages should not contain newline characters. The log message in line 214 is much better.

Cosmetic

OK

205

throws after method and javadoc

Cosmetic

OK

207

Should this be implemented now?

Minor

REJECT

Comments on file 'trunk/src/dk/netarkivet/archive/arcrepository/bitpreservation/AdminDataMessage.java', revision 1150

Lines

Description

Classification

Status

91

Java doc beside @return

Cosmetic

OK

128

Java doc beside @return

Cosmetic

OK

IssuesFromNs125 (last edited 2010-08-16 10:24:10 by localhost)