You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mathiasschw-db (via GitHub)" <gi...@apache.org> on 2024/07/02 17:01:14 UTC
[PR] Adding tests, structure, best practices to productionize code base [spark-connect-go]
mathiasschw-db opened a new pull request, #20:
URL: https://github.com/apache/spark-connect-go/pull/20
### What changes were proposed in this pull request?
This change contains several improvements that all aim to increase the code quality of the spark-connect-go repo. The changes fall in these categories:
- Improve unit test coverage by about 30 percentage points
- Decoupled the components in the sql package to make them individually testable and only depend on each others interfaces rather than implementation
- Added context propagation to the code base. This allows users of the library to set connection timeouts, auth headers etc.
- Added method/function level comments where they were missing for public functions
- Removed the global var builder 'entry point' and replaced it by a normal constructor so that each builder is simply new instead of the previous copy semantics
- Added a simple error hierarchy so that errors can be handled by looking at error types instead of just string values
- Removed a strange case of panic'ing the the whole process if some input was invalid
- Updated documentation and examples to reflect these changes
### Why are the changes needed?
These changes aim (along with subsequent changes) to get this code base to a point where it will eventually be fit for production use, something that is strictly forbidden right now
### Does this PR introduce _any_ user-facing change?
The PR as much as possible aims to not change the API but in a few cases this has not been possible. In particular, functions that eventually result in an outbound call to GRPC now take a context parameter. This is necessary and required for real production grade code. In addition, the builder is instantiated slightly differently (actually instantiated instead of being a global var) but the API for it otherwise remains.
### How was this patch tested?
All the code that was touch, has gotten some degree of unit testing that at least ensures coverage as well as checking of output
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Adding tests, structure, best practices to productionize code base [spark-connect-go]
Posted by "mathiasschw-db (via GitHub)" <gi...@apache.org>.
mathiasschw-db commented on code in PR #20:
URL: https://github.com/apache/spark-connect-go/pull/20#discussion_r1657136729
##########
go.mod:
##########
@@ -15,7 +15,7 @@
module github.com/apache/spark-connect-go/v1
-go 1.19
+go 1.21
Review Comment:
This is not strictly necessary for the code itself but some of the `error.Is` functionality will not work in 1.19. It will compile fine and work with whatever features existed in 1.19 though
##########
go.mod:
##########
@@ -15,7 +15,7 @@
module github.com/apache/spark-connect-go/v1
-go 1.19
+go 1.21
Review Comment:
This is not strictly necessary for the code itself but some of the `error.Is` functionality will not work in 1.19. It will compile fine and work with whatever features existed in 1.19 though. Our tests assume (and check) 1.21 functionality
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Adding tests, structure, best practices to productionize code base [spark-connect-go]
Posted by "ptxmac (via GitHub)" <gi...@apache.org>.
ptxmac commented on code in PR #20:
URL: https://github.com/apache/spark-connect-go/pull/20#discussion_r1657123825
##########
client/sparkerrors/errors.go:
##########
@@ -0,0 +1,48 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package sparkerrors
+
+import (
+ "errors"
+ "fmt"
+)
+
+type wrappedError struct {
+ errorType error
+ cause error
+}
+
+func (w *wrappedError) Unwrap() []error {
+ return []error{w.errorType, w.cause}
+}
+
+func (w *wrappedError) Error() string {
+ return fmt.Sprintf("%s: %s", w.errorType, w.cause)
+}
+
+func WithType(err error, errType errorType) error {
Review Comment:
Add a comment
```suggestion
// WithType wraps an error with a type that can later be checked using `errors.Is`
func WithType(err error, errType errorType) error {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] Adding tests, structure, best practices to productionize code base [spark-connect-go]
Posted by "mathiasschw-db (via GitHub)" <gi...@apache.org>.
mathiasschw-db commented on code in PR #20:
URL: https://github.com/apache/spark-connect-go/pull/20#discussion_r1657096901
##########
client/channel/channel.go:
##########
@@ -35,22 +37,22 @@ import (
// Reserved header parameters that must not be injected as variables.
var reservedParams = []string{"user_id", "token", "use_ssl"}
-// The ChannelBuilder is used to parse the different parameters of the connection
+// Builder is used to parse the different parameters of the connection
// string according to the specification documented here:
//
// https://github.com/apache/spark/blob/master/connector/connect/docs/client-connection-string.md
-type ChannelBuilder struct {
+type Builder struct {
Review Comment:
This avoid the stuttering interface that it had before (channel.ChannelBuilder, now channel.Builder). The latter is considered best practice Go
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org