AzCopy Code Review #1

prjain-msft · 2018-02-09T23:49:15Z

No description provided.

…tadata handling done at the backend

zezha-msft · 2018-02-12T08:09:21Z

Please consider squashing some of these commits together, so that it's easier to review each concrete change. 😃

JeffreyRichter · 2018-02-12T16:56:14Z

cmd/cancel.go

+	cancelCmd := &cobra.Command{
+		Use:        "cancel",
+		SuggestFor: []string{"cancl", "ancl", "cacl"},
+		Short:      "cancel cancels the existing job for given JobId",


cancel cancels an existing job

JeffreyRichter · 2018-02-12T16:56:28Z

cmd/cancel.go

+		Use:        "cancel",
+		SuggestFor: []string{"cancl", "ancl", "cacl"},
+		Short:      "cancel cancels the existing job for given JobId",
+		Long:       `cancel cancels the existing job for given JobId`,


cancel cancels an existing job

JeffreyRichter · 2018-02-12T17:05:14Z

cmd/cancel.go

+		Short:      "cancel cancels the existing job for given JobId",
+		Long:       `cancel cancels the existing job for given JobId`,
+		Args: func(cmd *cobra.Command, args []string) error {
+			// the cancel command requires necessarily to have an argument


// the cancel command requires a JobId argument; it then cancels all parts of the specified job.

JeffreyRichter · 2018-02-12T17:06:29Z

cmd/cancel.go

+
+			// If no argument is passed then it is not valid
+			if len(args) != 1 {
+				return errors.New("this command only requires jobId")


We should talk about how errors are presented to the user. This is fine for now, but we can't ship like this.

JeffreyRichter · 2018-02-12T17:08:20Z

cmd/copy.go

 		SuggestFor: []string{"cpy", "cy", "mv"}, //TODO why does message appear twice on the console
-		Short: "copy(cp) moves data between two places.",
+		Short:      "copy(cp) moves data between two places.",


You can't say "copy moves" as this indicates that copy doesn't "copy" and that it "moves" instead. At some point, we should get Sercan and go through all of this. Maybe we also get Roy, the doc guy to work with us on this.

JeffreyRichter · 2018-02-12T17:10:59Z

cmd/pause.go

+	pauseCmd := &cobra.Command{
+		Use:        "pause",
+		SuggestFor: []string{"pase", "ause", "paue"},
+		Short:      "pause pauses the existing job for given JobId",


You're missing an article in these sentences: "pause pauses the existing job for the specified JobId"

JeffreyRichter · 2018-02-12T17:21:33Z

cmd/validators.go

 	"github.com/Azure/azure-storage-azcopy/common"
+	"net/url"
+	"os"
 )

 func determineLocaltionType(stringToParse string) common.LocationType {


Why is this method lowercase (private) and IsLocalPath & IsURL uppercase (public)?
If the 2 IsXxx methods are only ever called by determineLocationType (spelled wrong, by the way), then define them inside the determineLocationType function itself.

JeffreyRichter · 2018-02-12T17:23:07Z

cmd/validators.go

@@ -42,14 +42,14 @@ func IsLocalPath(path string) bool {
 	_, err := os.Stat(path)
 	// in case the path does not exist yet, an err is returned
 	// we need to make sure that it is indeed just a local path that does not exist yet, and not a url
-	if err == nil || (!IsUrl(path) && os.IsNotExist(err)){
+	if err == nil || (!IsUrl(path) && os.IsNotExist(err)) {


You could just "return err = nil || (!IsURL(path) && os.IsNotExist(err)" and then kill the braces and the return true & return false

JeffreyRichter · 2018-02-12T17:26:58Z

cmd/validators.go

@@ -61,4 +61,4 @@ func IsUrl(givenUrl string) bool{
 		return false


You could just return !(u.Host == "" || u.Scheme == "" || u.Path == "")

JeffreyRichter · 2018-02-12T17:30:40Z

common/frontend-ste-models.go

 )
-type JobID string   //todo -- to uuid
+
+type JobID UUID


Let be consistent everywhere with casing: JobID or JobId -- I think JobID is better (as per http://www.dictionary.com/browse/id?s=t).

JeffreyRichter · 2018-02-12T17:31:54Z

common/frontend-ste-models.go

+
+type JobID UUID
+
+func (j JobID) String() (string){


Is this function necessary? Maybe if JobID doesn't have String, it will automatically call String on the UUID type? I'm not sure myself.

JeffreyRichter · 2018-02-12T17:34:18Z

common/frontend-ste-models.go

+type TransferStatus uint32
+
+func (status TransferStatus) String() (statusString string) {
+	switch uint32(status) {


Get rid of the switch cast and integers in the cases -- use case with constants instead.
If you are defining a data type (like TransferStatus), then use that data type; avoid casting it away. Casting it away defeats the whole purpose.

JeffreyRichter · 2018-02-12T17:45:17Z

common/frontend-ste-models.go

+func (status TransferStatus) String() (statusString string) {
+	switch uint32(status) {
+	case 0:
+		return "InProgress"


Where do these strings appear? Are they customer facing? logging? Both? If customer facing, then I'm sure we don't want something like "TransferComplete" without a space.

JeffreyRichter · 2018-02-12T17:48:00Z

common/frontend-ste-models.go

+	// Transfer has failed due to some error. This status does represent the state when transfer is cancelled
+	TransferFailed TransferStatus = 2
+
+	// Transfer is any of the three possible state (InProgress, Completer or Failed)


When is this used again? It is very strange to me. Also TransferStatus is a uint32 now not a byte so maybe we want a different value here? Maybe we should do this a different way?

JeffreyRichter · 2018-02-12T17:50:12Z

common/frontend-ste-models.go

+	TransferAny TransferStatus = TransferStatus(254)
+)
+
+func TransferStatusStringToCode(statusString string) TransferStatus {


Why do we need this? If we really do need it, then the strings should be defined once as constants and re-used otherwise a spelling change in one place breaks the other place. Another way to go is to initialize a map[status]string and initialize it with number/string pairs. Then, getting the string from number is trivial and getting the number from string requires a for range loop which is slower but probably less commonly done.

JeffreyRichter · 2018-02-12T17:51:21Z

common/frontend-ste-models.go

+}
+
+type LogLevel pipeline.LogLevel
+const (


There is no need for any of this; just use pipeline.LogLevel and its constants directly. Also, why have String()?

JeffreyRichter · 2018-02-12T17:55:14Z

common/frontend-ste-models.go

@@ -37,7 +129,7 @@ type CopyCmdArgsAndFlags struct {
 	Destination string

 	// inferred from arguments
-	SourceType LocationType
+	SourceType      LocationType


I put the comment here because github won't let me put the comment on line 146. I think we want to define a type Metadata string for Metadata when used in a JobPartPlan. Then, you can easily add a method that converts it to an azblob.Metadata type so it can be passed into PutBlob/PutBlock method.

JeffreyRichter · 2018-02-12T17:56:38Z

common/frontend-ste-models.go

@@ -63,15 +155,16 @@ type CopyCmdArgsAndFlags struct {

 // ListCmdArgsAndFlags represents the raw list command input from the user
 type ListCmdArgsAndFlags struct {
-	JobId		string
-	TransferStatus string
+	JobId    string


Should this be a UUID? In the code you want to parse command line strings into specific data types as soon as possible to get the value of type safety in your code base as quickly as possible.

JeffreyRichter · 2018-02-12T17:57:01Z

common/frontend-ste-models.go

-	JobId		string
-	TransferStatus string
+	JobId    string
+	OfStatus string


This should be a specific data type too, right?

…code; changes in get job summary

…stack support

…policy

…ring'

…d enumeration

…xecuting chunk func. the changes are added only for debug purpose #1

This reverts commit 0046ccd.

prjain-msft added 8 commits February 7, 2018 23:34

code refactor changes

4e74d89

comments added

5c72ec4

code refactor

d8f5656

copyright header added

f8e7457

code review comments

f79f74a

code review changes

f4e728f

code refactor, content-encoding, content-type, guess-mime-type and me…

fb67ed2

…tadata handling done at the backend

common logger added

eebb8d4

prjain-msft requested review from JeffreyRichter and zezha-msft February 9, 2018 23:49

JeffreyRichter reviewed Feb 12, 2018

View reviewed changes

prjain-msft added 22 commits May 7, 2018 18:09

fixed rpc enum

8e3dddb

fixed the rm; fixed the blob tier failure; change in transfer status …

a7467c1

…code; changes in get job summary

fixed the blob tier enum and check for blob tier while uploading

6d0662d

force flag added to skip overwriting existing blob

fce543e

test case added for force flag

1563fa7

code review changes

e30fec0

lot of minor changes to address comments in the code review

7521ce2

code to change the service version for azure stack support

8488c00

code review changes for code to change the service version for azure …

51d973b

…stack support

closing the file immediately memory mapping it; changes in the retry …

fa8d75e

…policy

closing the file immediately after memory mapping it

6002c10

support added to remove files; force flag feature added for file share

725d1e4

added support for canceling job by writing cancel on stdin

120ba2a

log level input changed to string

0e3297b

fixed test cases for the recent change 'log level input changed to st…

843c380

…ring'

enum added for log level

bde7bac

fixed the test_force_flag_set_to_false_upload test case

e3325b9

temp front-end changes

1bcec9c

readstdinforcancel changes; log enum changes; changes in copy downloa…

c17d898

…d enumeration

fixed compilation errors

12ad800

code refactored in copydownloadblobenumerator

8469511

fixed blobNameMatchesThePattern for linux

cfa1961

prjain-msft added a commit that referenced this pull request Jun 22, 2018

added functions to monitor the active routines doing the transfer / e…

00643db

…xecuting chunk func. the changes are added only for debug purpose #1

zezha-msft closed this Jun 27, 2018

prjain-msft added a commit that referenced this pull request Aug 20, 2018

"Fixing CI Build for darwin amd_64 #1"

622090e

This reverts commit 0046ccd.

adreed-msft mentioned this pull request Oct 4, 2019

Final set of edits #643

Merged

abrahaej mentioned this pull request Apr 11, 2020

Feature to change throughput rate on the fly #956

Closed

emul8ed mentioned this pull request Apr 21, 2020

SIGBUS error in TransferStatus on aarch64 #972

Closed

NicholasDawson mentioned this pull request Jul 18, 2023

azcopy login fails silently #1805

Closed

marc-hb mentioned this pull request Aug 10, 2024

azcopy list - show only top level directories #858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AzCopy Code Review #1

AzCopy Code Review #1

prjain-msft commented Feb 9, 2018

zezha-msft commented Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018 •

edited

Loading

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

JeffreyRichter Feb 12, 2018

		@@ -61,4 +61,4 @@ func IsUrl(givenUrl string) bool{
		return false

AzCopy Code Review #1

AzCopy Code Review #1

Conversation

prjain-msft commented Feb 9, 2018

zezha-msft commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffreyRichter Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffreyRichter Feb 12, 2018 •

edited

Loading