Skip to content

lint fixes over initial orc import#68

Closed
sougou wants to merge 5 commits intoss-oc0-initial-importfrom
ss-oc1-initial-orc
Closed

lint fixes over initial orc import#68
sougou wants to merge 5 commits intoss-oc0-initial-importfrom
ss-oc1-initial-orc

Conversation

@sougou
Copy link
Copy Markdown

@sougou sougou commented Aug 23, 2020

No description provided.

sougou added 5 commits August 21, 2020 20:01
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
There is currently no plan to use raft. So, we should eventually
delete this entire dependency.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Copy link
Copy Markdown

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the deal with all the JS/templates "added files"?

This entire PR seems just like automated linter with no actual changes. @sougou I think this is not the PR we wanted?

log.Fatale(err)
}
fmt.Println(fmt.Sprintf("%s<%s", instanceKey.DisplayString(), destinationKey.DisplayString()))
fmt.Printf("%s<%s\n", instanceKey.DisplayString(), destinationKey.DisplayString())
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, were these changes automated by some tool? If so, which and how?

c.Lock()
defer c.Unlock()
c.Lock() //nolint SA5011: possible nil pointer dereference
defer c.Unlock() //nolint SA5011: possible nil pointer dereference
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible nil pointer dereference

incorrect. bad linter ☝️

func isInMemorySQLite() bool {
return config.Config.IsSQLite() && strings.Contains(config.Config.SQLite3DataFile, ":memory:")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would encourage to keep this function. True, it's not used in today's code, but it may be used in tomorrow's code.

return
}
instanceKey, err := this.getInstanceKey(params["host"], params["port"])
instanceKey, _ := this.getInstanceKey(params["host"], params["port"])
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter should have suggested to check the err value instead of completely muting it.

ErrantGTIDStructureWarning StructureAnalysisCode = "ErrantGTIDStructureWarning"
NoFailoverSupportStructureWarning StructureAnalysisCode = "NoFailoverSupportStructureWarning"
NoWriteableMasterStructureWarning StructureAnalysisCode = "NoWriteableMasterStructureWarning"
NotEnoughValidSemiSyncReplicasStructureWarning StructureAnalysisCode = "NotEnoughValidSemiSyncReplicasStructureWarning"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

distributionReentry int64
client *consulapi.Client
kvCache *cache.Cache
distributionReentry int64
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

CoMasterRecovery = "CoMasterRecovery"
IntermediateMasterRecovery = "IntermediateMasterRecovery"
CoMasterRecovery RecoveryType = "CoMasterRecovery"
IntermediateMasterRecovery RecoveryType = "IntermediateMasterRecovery"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

MasterRecoveryBinlogServer = "MasterRecoveryBinlogServer"
MasterRecoveryGTID MasterRecoveryType = "MasterRecoveryGTID"
MasterRecoveryPseudoGTID MasterRecoveryType = "MasterRecoveryPseudoGTID"
MasterRecoveryBinlogServer MasterRecoveryType = "MasterRecoveryBinlogServer"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const (
OrchestratorExecutionCliMode OrchestratorExecutionMode = "CLIMode"
OrchestratorExecutionHttpMode = "HttpMode"
OrchestratorExecutionHttpMode OrchestratorExecutionMode = "HttpMode"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

discover(hostname, mySQLPort)
});

});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what;s this all about?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should have been part of the initial import, which I later added...

Copy link
Copy Markdown
Author

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw your comments. Unfortunately it was after merging the other PR. But we can fix forward as needed.

discover(hostname, mySQLPort)
});

});
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should have been part of the initial import, which I later added...

@sougou
Copy link
Copy Markdown
Author

sougou commented Aug 23, 2020

The fixes were made by me manually. I started adding those additional error checks, but got cold feet because they could change existing behavior. So, I erred on the side of preserving existing behavior.

@sougou sougou closed this Aug 24, 2020
@sougou sougou deleted the ss-oc1-initial-orc branch August 24, 2020 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants