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

optimizations/refactoring #299

Merged
merged 45 commits into from
Jan 9, 2019
Merged

optimizations/refactoring #299

merged 45 commits into from
Jan 9, 2019

Conversation

Lagovas
Copy link
Collaborator

@Lagovas Lagovas commented Jan 9, 2019

  • added interface for Proxy that process mysql/postgresql connections and unified signatures for mysql/postgresql proxy methods to be compatible with one interface
  • added interface for decryptors and unified signatures for db specific decryptors
  • added decryptor/proxy factories that initialized at start of program and then used after accepting new connections. old code did initialization of components in different steps of processing connections (it was from our first version of server and need refactoring)
  • initialize all db specific settings and components in main function once
  • dropped redundant and unused fields/methods/constants in project (found them when tested golangci-lint : )
  • refactored and encapsulated switching to tls in postgresql proxy (it was need to unifying usage of abstract proxy component without postgresql specific logic)
  • optimized OnQueryObserver logic to re-use parsed queries between several observers (to avoid several parsing query, reuse ast tree and serialize to string only at end)
  • add using absolute paths in integration tests to allow re-use in another projects/repos
  • make some methods in our default keystore public to use in enterprise version

As I remember, I didn't change any public CLI or API interfaces that should be changed in documentation. And didn't change any behavior of server. Touched only some log messages (uppercase or change level from DEBUG to INFO)

Copy link
Contributor

@shadinua shadinua left a comment

Choose a reason for hiding this comment

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

Really deep refactoring :-) Great!

Copy link
Collaborator

@vixentael vixentael left a comment

Choose a reason for hiding this comment

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

Great job!

It's very useful to have unified API of decryptors and Proxy.

@@ -143,9 +143,15 @@ func (ex *WriteToFileExecutor) Execute(data []byte) {

// Close file
func (ex *WriteToFileExecutor) Close() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

if *host != cmd.DEFAULT_ACRA_HOST || *port != cmd.DEFAULT_ACRASERVER_PORT {
*acraConnectionString = network.BuildConnectionString("tcp", *host, *port, "")
config.SetAcraConnectionString(network.BuildConnectionString("tcp", *host, *port, ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

poisonCallbacks.AddCallback(base.NewExecuteScriptCallback(*scriptOnPoison))
config.scriptOnPoison = *scriptOnPoison
}
// must be last
Copy link
Collaborator

Choose a reason for hiding this comment

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

"must be last" in terms of setting poison callbacks?
maybe it makes sense to change to "should setup "stopOnPoison" as last poison record callback"

cmd/acra-server/acra-server.go Outdated Show resolved Hide resolved
cmd/acra-server/acra-server.go Outdated Show resolved Hide resolved
}

// WithZoneID replace ZoneID and return itself
func (ctx *DataProcessorContext) WithZoneID(id []byte) *DataProcessorContext {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would expect that ctx.WithZoneID() will return new copy of itself with selected ZoneID. Maybe it's only me, but maybe it makes sense to rename method to SetZoneID / UpdateZoneID? To make clear that it returns the same context, just ZoneID updated.

}

// WithContext replace context and return itself
func (ctx *DataProcessorContext) WithContext(newContext context.Context) *DataProcessorContext {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

@@ -128,7 +128,7 @@ func (packet *MysqlPacket) replaceQuery(newQuery string) {

// readPacket read header to struct and return payload as return result or error
func (packet *MysqlPacket) readPacket(connection net.Conn) ([]byte, error) {
if _, err := connection.Read(packet.header); err != nil {
if _, err := io.ReadFull(connection, packet.header); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the difference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

helper read more than once (if first read call was not full and return only part of data cached in os buffer). until some error or until no data was returned. ReadFull read in loop until read expected num of bytes. I meet one failed test when here read return part of packet and without error

decryptor/postgresql/proxy.go Outdated Show resolved Hide resolved
)

var lock = sync.RWMutex{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

bye-bye mutex 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it wasn't used)

@Lagovas Lagovas merged commit c16e7f4 into cossacklabs:master Jan 9, 2019
@Lagovas Lagovas deleted the lagovas/open-source-version branch January 15, 2019 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants