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

rest: fix error code #190

Merged
merged 2 commits into from
Mar 26, 2021
Merged

rest: fix error code #190

merged 2 commits into from
Mar 26, 2021

Conversation

hackaugusto
Copy link
Contributor

it should be 40403 and not 40401

it should be 40403 and not 40401
@hackaugusto hackaugusto requested a review from a team March 25, 2021 12:17
Copy link
Contributor

@tvainika tvainika left a comment

Choose a reason for hiding this comment

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

There are set of error codes in karapace/kafka_rest_apis/error_codes.py here https://github.com/aiven/karapace/blob/09d426d1db3a41e7009371abffb5da77311e142f/karapace/kafka_rest_apis/error_codes.py#L10

I think using those enum error codes would make sense here, also this PR should explain why 40403 is correct and not 40401 for referencing purposes.

@hackaugusto
Copy link
Contributor Author

I think using those enum error codes would make sense here

I agree, but ATM I'm not sure how to resolve the naming conflict. The existing names for the error codes were based on how Karapace was using these codes, now we have a CONSUMER_NOT_FOUND with the value 40403:

https://github.com/aiven/karapace/blob/09d426d1db3a41e7009371abffb5da77311e142f/karapace/kafka_rest_apis/error_codes.py#L12

However that name does not make sense in the context of a UnknownTopicOrPartitionError.

PR should explain why 40403 is correct and not 40401 for referencing purposes.

Ran the test using --registry-url http://localhost:8081 --rest-url http://localhost:8082/ against a schema registry node.

HelenMel
HelenMel previously approved these changes Mar 26, 2021
@tvainika
Copy link
Contributor

I agree, but ATM I'm not sure how to resolve the naming conflict. The existing names for the error codes were based on how Karapace was using these codes, now we have a CONSUMER_NOT_FOUND with the value 40403:

However that name does not make sense in the context of a UnknownTopicOrPartitionError.

Would it make sense to remove @unique and just define UNKNOWN_TOPIC_OR_PARTITION = 40403?

PR should explain why 40403 is correct and not 40401 for referencing purposes.

Ran the test using --registry-url http://localhost:8081 --rest-url http://localhost:8082/ against a schema registry node.

Yes, I can understand it should be same as Confluent's Schema Registry, but do we have any reference or specifications to follow? Probably not that nor automated test suite to compare.

@hackaugusto
Copy link
Contributor Author

Yes, I can understand it should be same as Confluent's Schema Registry, but do we have any reference or specifications to follow?

AFAIK only the HTTP response codes are documented. https://docs.confluent.io/platform/current/schema-registry/develop/api.html#subjects

Probably not that nor automated test suite to compare

I'm not sure if that is what you meant, but I used our integration tests to compare our expected results against their API. Unfortunately we are most likely not covering every case, but that is what caught the difference here.

Would it make sense to remove @unique and just define UNKNOWN_TOPIC_OR_PARTITION = 40403?

sure, will do :)

@@ -199,7 +199,7 @@ async def test_topics(rest_async_client, admin_client):
assert data["partitions"][0]["replicas"][0]["in_sync"], "Replica should be in sync"
res = await rest_async_client.get(f"/topics/{topic_foo}")
assert res.status_code == 404, f"Topic {topic_foo} should not exist, status_code={res.status_code}"
assert res.json()["error_code"] == 40401, "Error code does not match"
assert res.json()["error_code"] == 40403, "Error code does not match"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to update the whole test_rest.py right now, there is lots of places using the hardcoded constant and not the enum there.

@tvainika tvainika merged commit e18a059 into master Mar 26, 2021
@tvainika tvainika deleted the fix-rest-get-topics-suberror-code branch March 26, 2021 09:24
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.

3 participants