New Backend: Apache Ignite #146

guscarreon · 2023-08-23T17:05:37Z

No description provided.

AlexBVolcy · 2023-08-23T19:02:56Z

backends/config/config_test.go

@@ -155,6 +155,13 @@ func TestNewBaseBackend(t *testing.T) {
 				{msg: "Error creating Redis backend: ", lvl: logrus.FatalLevel},
 			},
 		},
+		//{


Can this commented out code be deleted?

It probably needs to be uncommented, and perhaps worked on if it isn't in working condition.

AlexBVolcy · 2023-08-23T19:13:41Z

backends/ignite.go

+		return "", err
+	}
+
+	// Unmarshall response


Very Minor Nitpick: Spelling Unmarshall --> Unmarshal

AlexBVolcy · 2023-08-23T19:16:07Z

backends/ignite.go

+		return "", utils.NewPBCError(utils.GET_INTERNAL_SERVER, igniteResponse.Error)
+	} else if igniteResponse.Status > 0 {
+		return "", utils.NewPBCError(utils.GET_INTERNAL_SERVER, "Ignite response.Status not zero")
+	} else if len(igniteResponse.Response) == 0 { // both igniteResponse.Status == 0 && len(igniteResponse.Error) == 0


I think you can delete the comment here

AlexBVolcy · 2023-08-23T19:16:26Z

backends/ignite.go

+		return false, err
+	}
+
+	// Unmarshall response


Same spelling nitpick

AlexBVolcy · 2023-08-23T19:17:23Z

backends/ignite.go

+	}
+
+	// Validate response
+	if igniteResponse.Status > 0 || len(igniteResponse.Error) > 0 {


Can this be combined into igniteResponse.Response like you did in getResponse validation?

This validation changed

AlexBVolcy · 2023-08-23T19:17:35Z

backends/ignite.go

+
+// NewIgniteBackend expects a valid config.IgniteBackend object
+func NewIgniteBackend(cfg config.Ignite) *IgniteBackend {
+


Nitpick: Can you remove this blank line

AlexBVolcy · 2023-08-23T19:18:00Z

backends/ignite.go

+// Get creates an Get http.Request with the URL query values needed to perform a "get" operation
+// on an Ignite storage instance using the REST API
+func (back *IgniteBackend) Get(ctx context.Context, key string) (string, error) {
+


Nitpick: Can you remove this blank line

AlexBVolcy · 2023-08-23T19:18:46Z

backends/ignite.go

+
+// Get creates an Get http.Request with the URL query values needed to perform a "get" operation
+// on an Ignite storage instance using the REST API
+func (back *IgniteBackend) Get(ctx context.Context, key string) (string, error) {


I think back can be renamed to ig, what do you think?

AlexBVolcy · 2023-08-23T19:19:58Z

utils/errors.go

@@ -18,6 +18,7 @@ const (
 	KEY_NOT_FOUND                    // GET http.StatusNotFound 404
 	KEY_LENGTH                       // GET http.StatusNotFound 404
 	UNKNOWN_STORED_DATA_TYPE         // GET http.StatusInternalServerError 500
+	GET_INTERNAL_SERVER              // PUT http.StatusInternalServerError 500


Should the comment here say // GET http.StatusInternalServerError 500?

hhhjort · 2023-08-24T19:24:43Z

backends/ignite.go

+
+// Get implements the IgniteDB interface Get method and makes the Ignite storage client retrieve
+// the value that has been previously stored under 'key' if its TTL is still current. We can tell
+// when a key is not faound when Ignite doesn't return an error, nor a 'Status' different than zero, but


faound -> found

hhhjort · 2023-10-06T15:33:41Z

backends/ignite.go

+
+// putResponse is used to unmarshal the Ignite server's response to a PUT request with
+// the "cmd" URL query field set to "putifabs"
+type putResponse struct {


Is there a reason for defining both a getResponse and a putResponse when they both seem to be the same structure?

The difference is that in putResponse.Response is a bool and getResponse.Response is a string. Do you want me to try to consolidate them into a single struct?

Good observation by @hhhjort but I think keeping them as separate structs due to the type difference on Response makes sense.

hhhjort · 2023-10-06T15:39:49Z

config/backends.go

+	Scheme  string            `mapstructure:"scheme"`
+	Host    string            `mapstructure:"host"`
+	Port    int               `mapstructure:"port"`
+	Secure  bool              `mapstructure:"secure"`


I think this should be renamed VerifyCert or some documentation added to explain that secure in this context means verifying the SSL certificate on the ignite server. On the face of it Secure sounds like it may be redundant with Scheme, or suggest that there is a third possibility for Scheme that may or may not use a secure connection.

Modified and added a comment

bsardo · 2023-10-24T19:21:13Z

config.yaml

+    host: "127.0.0.1"
+    port: 8080
+    secure: false
+    headers: !!omap


Is !!omap needed here? It looks like this is just to ensure order is preserved when loading the file but there is only one element.

I believe we need it to read the headers as map[String]string

bsardo · 2023-10-24T20:47:24Z

config/backends.go

+	log.Infof("config.backend.ignite.host: %s", cfg.Host)
+	log.Infof("config.backend.ignite.port: %d", cfg.Port)
+	log.Infof("config.backend.ignite.cache.name: %s", cfg.Cache.Name)
+	log.Infof("config.backend.ignite.cache.create_on_start: %v", cfg.Cache.CreateOnStart)


Is this supposed to be %t instead of %v?

bsardo · 2023-10-24T20:50:52Z

config/backends.go

+	VerifyCert bool              `mapstructure:"secure"`
+	Headers    map[string]string `mapstructure:"headers"`


These don't have log.Infof statements in validateAndLog. I'm not sure if that's intentional.

bsardo · 2023-10-25T16:44:50Z

backends/ignite.go

+	}
+
+	if httpResp.StatusCode != http.StatusOK {
+		httpErr = fmt.Errorf("Ignite error. Unexpected status code: %d", httpResp.StatusCode)


Are we able to just return here when this error is detected?

If we return here, we'd get an entry in the logs with the status code alone, which is not that helpful. The logic behind not exiting here is to append more information to httpErr such as the error message found in httpResp.Body or lack thereof.

I see. I can get on board with this approach, however, I'm curious if/when the additional details are needed. For example, typically if the status code is 204 that means that the request was fulfilled without a body so if that is a status code that Ignite might send back, I don't see why we might need to append "Received empty httpResp.Body" to the error message. Shouldn't we be able to just report back that status code 204 was received from Ignite and rely on that status code's meaning to tell the story?

I guess I'm asking if you've observed a need to provide this additional information when the status code is not 200 when working with a live Ignite server. In most cases the semantics of a HTTP code provide the information we need. Furthermore, I'm curious if you've observed any instances where Ignite provides an error message in the body along with the non-200 status code. If so, would that suffice?

Discussed offline. The Ignite REST API docs imply that all responses are 200s and we have yet to encounter a non-200 response when working with a live Ignite server. Any error details are expected to be in the response body. As such, I'm good with the current design which is focused on receiving and unpacking a response while catching transport and response body errors but also accounting for the possibility of non-200 error codes. A non-200 error code is returned but only after we've attempted to read the response body since there might be more information there.

bsardo · 2023-10-25T16:47:44Z

backends/ignite.go

+	if httpResp.Body == nil {
+		errMsg := "Received empty httpResp.Body"
+		if httpErr == nil {
+			return nil, fmt.Errorf("Ignite error. %s", errMsg)
+		}
+		return nil, fmt.Errorf("%s; %s", httpErr.Error(), errMsg)
+	}


If we decide it makes sense to return on line 59 above then I think this logic can be simplified to:

if httpResp.Body == nil { return nil, errors.New("Ignite error. Received empty body") }

I was thinking about keeping the logic above in order to potentially have more helpful error logs. Do you agree?

bsardo · 2023-10-25T18:33:25Z

backends/ignite.go

+	igniteResponse := getResponse{}
+
+	if unmarshalErr := json.Unmarshal(responseBytes, &igniteResponse); unmarshalErr != nil {
+		return fmt.Errorf("Unmarshal response error: %s; Response body: %s", unmarshalErr.Error(), string(responseBytes))


Just curious, is this error message style consistent with the rest of PBC? This error would ultimately be logged as "Error creating Ignite backend: Unmarshal response error: [some error] Response body: [body bytes]" by the calling code.

It's kind of long but, this is the only Unmarshal error that PBC handles. We are currently using Ignite error for server side errors, validation errors look like: Error creating Ignite backend, so Unmarshal response error seemed reasonable for me. Should we keep messages shorter?

bsardo · 2023-10-25T19:00:28Z

backends/ignite.go

+	igniteResponse := getResponse{}
+
+	if unmarshalErr := json.Unmarshal(responseBytes, &igniteResponse); unmarshalErr != nil {
+		return "", fmt.Errorf("Unmarshal response error: %s; Response body: %s", unmarshalErr.Error(), string(responseBytes))


Should an unmarshal error be considered to be of type utils.GET_INTERNAL_SERVER?

Good question. GET_INTERNAL_SERVER and PUT_INTERNAL_SERVER are being used to label errors that happen on the backend side. Given that this particular error happens on PBC, I decided to keep things simple and not label this error as INTERNAL_SERVER but, I realize that a malformed JSON blob could be consequence of a server side error. Maybe a JSON validation error should be considered a GET_INTERNAL_SERVER error. Thoughts?

Hm I was thinking an unmarshal failure here can be attributed to a malformed response from the Ignite server, in which case I would consider this a GET_INTERNAL_SERVER error. I'm curious what @hhhjort thinks.

I think you are right. Modified

bsardo · 2023-10-25T21:15:54Z

backends/ignite.go

+
+// putResponse is used to unmarshal the Ignite server's response to a PUT request with
+// the "cmd" URL query field set to "putifabs"
+type putResponse struct {


Good observation by @hhhjort but I think keeping them as separate structs due to the type difference on Response makes sense.

bsardo · 2023-10-26T14:08:15Z

backends/ignite.go

+		return utils.NewPBCError(utils.PUT_INTERNAL_SERVER, "Ignite responded with non-zero successStatus code")
+	}
+
+	if !igniteResponse.Response {


Is the ! supposed to be here? If igniteResponse.Response is true a record already exists right?

Given that we are not rewritting values for existing keys, we are using Apache Ignite's putifabsent command. According to the put-if-absent docs: "The response field contains true if the entry was put, false otherwise."

bsardo · 2023-10-26T14:29:26Z

backends/ignite_test.go

+			desc: "config validation error",
+			testCases: []testCase{
+				{
+					desc: "empty scheme",


Nitpick: for the "empty scheme", "empty host" and "empty port" tests, I think it would be best if all of the values were valid except for a single field so we can be sure the field under test is what is causing the error. The error message is the same for all of these empty fields so it is not possible to discern what is causing the failure as the test is currently constructed.

hhhjort self-requested a review August 23, 2023 17:10

hhhjort self-assigned this Aug 23, 2023

hhhjort requested a review from AlexBVolcy August 23, 2023 17:12

hhhjort assigned AlexBVolcy Aug 23, 2023

AlexBVolcy reviewed Aug 23, 2023

View reviewed changes

hhhjort reviewed Aug 24, 2023

View reviewed changes

guscarreon added 10 commits September 26, 2023 13:30

New Backend: Apache Ignite

3c6006b

Create cache flag

3de5248

Error handling

d1672b1

Some tweaks

72bfb68

Added secure config flag

1f3df30

Refactored

38c4722

httpWrapper working

4b4a26d

First JSON test

812b406

Addressed review comments

79cf4fe

Rolled back config.yaml

6fb3224

guscarreon force-pushed the newBackendIgnite branch from efb169e to 6fb3224 Compare September 26, 2023 17:36

bsardo assigned bsardo and unassigned hhhjort Sep 28, 2023

init http.Header

370fed6

bsardo assigned hhhjort and unassigned bsardo Oct 5, 2023

hhhjort reviewed Oct 6, 2023

View reviewed changes

guscarreon added 2 commits October 9, 2023 12:37

HAns review and bug correction

c8090c9

corrected test cases

9c9057b

bsardo assigned bsardo and unassigned AlexBVolcy Oct 23, 2023

hhhjort previously approved these changes Oct 23, 2023

View reviewed changes

bsardo reviewed Oct 26, 2023

View reviewed changes

Brian's review

da51751

guscarreon dismissed hhhjort’s stale review via da51751 November 7, 2023 16:10

guscarreon added 2 commits November 7, 2023 12:05

TestIgniteGet correction

74f808b

Unmarshal error is an GET_INTERNAL_SERVER error

64d357e

bsardo approved these changes Dec 4, 2023

View reviewed changes

hhhjort approved these changes Dec 6, 2023

View reviewed changes

bsardo merged commit 58cffa3 into master Dec 6, 2023
3 checks passed

cross-repo-issue-creation bot mentioned this pull request Dec 6, 2023

Port PR from PBC-Go: New Backend: Apache Ignite prebid/prebid-cache-java#123

Closed


		// NewIgniteBackend expects a valid config.IgniteBackend object
		func NewIgniteBackend(cfg config.Ignite) *IgniteBackend {

		VerifyCert bool `mapstructure:"secure"`
		Headers map[string]string `mapstructure:"headers"`

New Backend: Apache Ignite #146

New Backend: Apache Ignite #146

Conversation

guscarreon commented Aug 23, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon Nov 6, 2023 • 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

guscarreon Nov 6, 2023 •

edited

Loading

guscarreon Nov 6, 2023 •

edited

Loading