Generatorfix #181
Generatorfix #181
Conversation
| import re | ||
|
|
||
| pattern = re.compile(r'chunked 1.0,(?P<metadata_length>\d+),(?P<body_length>\d+)\n') | ||
| pattern = re.compile(r'chunked 1.0,(?P<metadata_length>\d+),(?P<body_length>\d+)') |
There was a problem hiding this comment.
Should we make the newline optional at the end with \n??
|
|
||
| match = pattern.match(line) | ||
| if match is None: | ||
| continue |
There was a problem hiding this comment.
If we have this logic, the following assertIsNotNone is not needed
|
@ljiang1 I'm still kind of confused how this fixes the issue? |
|
@itay As Jacob suggested in previous pr, comparing to streamingcommand and other commands, before they call the customer supplied execution, they executed the searchcommands._records_protocol_v2(), which read the reply from splunk and check if the reply action is execute; but the generatingcommand didn't call _records_protocol_v2(). the fix is adding the same checking code as _records_protocol_v2(). The reason can't do the same calling format as streamingcommand is that the way of _records_protocol_v2() implements prevents the yielding result, so I end up just add the check code here. |
|
If the script is being invoked just for getinfo, splunkd will close the processes' stdin after it receives the getinfo reply, so the SDK should read an EOF. Since this change adds the _read_chunk to _execute, it will detect the EOF and return None and return early from _execute. Thus, the GeneratingCommand's generate() method is never called in the case that this is just an invocation for getinfo. I believe this change should address the immediate bug we're seeing (generate() method being invoked multiple times when it shouldn't be). I think there's a separate issue with the GeneratingCommand class in that it only ever emits one chunk (e.g. you couldn't have a generating command that generates an infinite stream of events). That's a pre-existing problem and doesn't need to be addressed as part of this bugfix. |
|
|
||
| if len(chunks) == 0: | ||
| self.assertEqual(ifile.readline(), '\n') # the getinfo exchange protocol requires this | ||
| # if len(chunks) == 0: |
There was a problem hiding this comment.
How come this is commented out? I'd prefer to not see new commented-out code committed to the repo.
leverich
left a comment
leverich
left a comment
There was a problem hiding this comment.
LGTM, except for the commented out code. Figure out if that needs to stay or go and then I'm happy.
| body="" | ||
| try: | ||
| body = ifile.read(body_length) | ||
| if body_length>0: |
| # if body_length <= 0: | ||
| # return metadata, '' | ||
|
|
||
| body="" |
itay
left a comment
itay
left a comment
There was a problem hiding this comment.
This looks good - can we squash these into one change and then @shakeelmohamed can merge?
4 participants
check if the action is execute before execute the generate command