-
Notifications
You must be signed in to change notification settings - Fork 102
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
Reduce logging #64
Reduce logging #64
Conversation
To avoid having files accidentally added to the project
Are you facing performance issues due to these logging statements? These statements are logged during auto-registration flow which we don't expect to be a frequent operation. Is it different in your use-case? |
See #63 🙂 – we get the log statement for every sent. Not sure of the performance implications, but drowns useful log messages and does have some log storage cost. |
@@ -284,7 +284,7 @@ public UUID createSchema(String schemaName, | |||
Map<String, String> metadata) throws AWSSchemaRegistryException { | |||
UUID schemaVersionId = null; | |||
try { | |||
log.info("Auto Creating schema with schemaName: {} and schemaDefinition : {}", schemaName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you seeing this logged for every request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only seeing the "Auto Creating schema" message once, and then the "Schema Version Id is null" on every message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this. Would you mind updating it in class GlueSchemaRegistryKafkaSerializer.java as well. This class is a generic class that can be used with additional property called "dataFormat" and passing "JSON" or "AVRO".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that re the information in #63, I think there might be an underlying issue, and changing the log level will just mask that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this got lost in merge conflicts resolution. This has been a miss on our side. The schema auto creation should still be info because we don't expect that to happen very often. The Over logging issue is legit. Are you suggesting that there is an underlying issue with the library or your set-up particularly ? Either ways, we would be happy to help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohitpali - so, if I understand correctly:
- Usecases where dynamic schema registration is used isn't meant to store the registered schemaVersionId, and the cache a crucial part of the design, not "just" an optimization.
- Usecases where you can manually evolve schemas can have the schemaVersionId predefined.
Even with re-changing log level to DEBUG, would it be worth adding a comment to the codebase that it's normal to get this on a per-message basis for dynamic scenarios, and that caching further down ensures this is not a performance problem?
Thanks for your help in clearing up matters! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your understanding is correct. Adding comment is also a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could please change the below line to info, we should be good to merge.
log.debug("Auto Creating schema with schemaName: {} and schemaDefinition : {}", schemaName, schemaDefinition);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could please change the below line to info, we should be good to merge.
@tveon is on vacation for the next few weeks, but I'll see if I can persuade him to do that one-line update :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. 👍 🌴
It looks like these changes were lost in a merge at some point. Created based on the diff on GitHub.
Hello, good people, do you have any plans for when the next release is going to be? 😃 PS: I just noticed that while there's a 1.1.1 release on Maven Central, this GitHub repo only has tags for 1.0.2 and 1.1.0. |
* Add .gitignore To avoid having files accidentally added to the project * Reduce and update logging It looks like these changes were lost in a merge at some point. Created based on the diff on GitHub.
* Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> * Protobuf support (#60) * Add message indices parser for Protobuf schemas. * Encode Protobuf messages with message index Co-authored-by: Ravindranath Kakarla <[email protected]> * Fix checkstyle build error and reduce tests for canaries * Update README.md * Update README.md * Pull everit from maven central instead of jitpack (#54) Co-authored-by: mohit <[email protected]> * Bump up version to v1.1.1 * Update ReadMe to v1.1.1 * Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> * Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> * Protobuf support (#60) * Add message indices parser for Protobuf schemas. * Encode Protobuf messages with message index Co-authored-by: Ravindranath Kakarla <[email protected]> * Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> * Update dependencies for Protobuf and create class structure cr: https://code.amazon.com/reviews/CR-52122396 * Add ProtobufMessageType to aid Protobuf deserialization cr: https://code.amazon.com/reviews/CR-54151956 * Protobuf DynamicMessage deserialization cr: https://code.amazon.com/reviews/CR-54424261 * Fix serializer build issue * Fix serializer build issue * Pull everit from maven central instead of jitpack (#54) Co-authored-by: mohit <[email protected]> * Fix resource clean up in Kafka integration test * Add documentation for configuring a specific GSR resource with the Kafka Connect converter (#58) * Reduce logging (#64) * Add .gitignore To avoid having files accidentally added to the project * Reduce and update logging It looks like these changes were lost in a merge at some point. Created based on the diff on GitHub. * Add DatumReader Cache to improve de-serialization performance (#65) * Add DatumReader Cache to improve de-serialization performance * Fix exception message Co-authored-by: Ravindranath Kakarla <[email protected]> * Introduce cache to improve serialization performance (#67) Co-authored-by: Ravindranath Kakarla <[email protected]> * Bump up version to v1.1.2 (#70) Co-authored-by: mohit <[email protected]> * Bump up v1.1.2 in README.md * Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object (#72) Co-authored-by: Ravindranath Kakarla <[email protected]> * Add a utility to derive class name from message descriptor (#74) * Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object * Add a utility to derive Protobuf compiled Java class name from FileDescriptor. Co-authored-by: Ravindranath Kakarla <[email protected]> * Add support to deserialize to Protobuf POJOs. (#76) Co-authored-by: Ravindranath Kakarla <[email protected]> * Handle ProtobufMessageType not set. (#85) Co-authored-by: Ravindranath Kakarla <[email protected]> * Introduce cache to improve Protobuf serializer/de-serializer performance. (#90) Co-authored-by: Ravindranath Kakarla <[email protected]> * Merging latest approved changes to protobuf-support branch. (#98) * Modify UserAgent to emit usage metrics (#77) * Add tests to include key and value schemas both (#79) Co-authored-by: Mohit Paliwal <[email protected]> * Bump to 1.1.3 (#78) * Modify UserAgent to emit usage metrics * Bump version to 1.1.3 Co-authored-by: Ravindranath Kakarla <[email protected]> * Update README.md * Upgrade Kafka version to 2.8.1 * Bump up version to 1.1.4 * Upgrade transitive dependencies to latest versions. Co-authored-by: Ravindranath Kakarla <[email protected]> * Remove configuration logging information (#89) * Bump up version to 1.1.5 (#91) * Specify the avro-maven-plugin version explicitly (#97) Co-authored-by: Ravindranath Kakarla <[email protected]> * Bump up version to v1.1.1 * Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Ravindranath Kakarla <[email protected]> Co-authored-by: Kexin Hui <[email protected]> Co-authored-by: Linyu Yao <[email protected]> * Add integration tests for Protobuf support (#99) * Protobuf integration tests for Kinesis use-cases * Protobuf integration test cases for Kafka use-cases. Co-authored-by: Ravindranath Kakarla <[email protected]> * Upgrade to latest versions of Protobuf dependencies. (#103) Co-authored-by: Ravindranath Kakarla <[email protected]> * Protobuf support (#117) * Modify UserAgent to emit usage metrics (#77) * Add tests to include key and value schemas both (#79) Co-authored-by: Mohit Paliwal <[email protected]> * Bump to 1.1.3 (#78) * Modify UserAgent to emit usage metrics * Bump version to 1.1.3 Co-authored-by: Ravindranath Kakarla <[email protected]> * Update README.md * Upgrade Kafka version to 2.8.1 * Bump up version to 1.1.4 * Upgrade transitive dependencies to latest versions. Co-authored-by: Ravindranath Kakarla <[email protected]> * Remove configuration logging information (#89) * Bump up version to 1.1.5 (#91) * Specify the avro-maven-plugin version explicitly (#97) Co-authored-by: Ravindranath Kakarla <[email protected]> * Fix bug with canary tests running the entire suite. (#102) Co-authored-by: Ravindranath Kakarla <[email protected]> * Upgrade to latest versions of Protobuf dependencies. * Bump log4j-core from 2.13.2 to 2.15.0 (#107) * Bump log4j-api from 2.13.2 to 2.15.0 (#106) Bumps log4j-api from 2.13.2 to 2.15.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump up version from 1.1.5 to 1.1.6 (#108) Co-authored-by: Mohit Paliwal <[email protected]> * Bump log4j-api from 2.15.0 to 2.16.0 (#109) Bumps log4j-api from 2.15.0 to 2.16.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump up version from 1.1.6 to 1.1.7 (#111) Co-authored-by: Mohit Paliwal <[email protected]> * Pull current region for integration tests (#112) Co-authored-by: Mohit Paliwal <[email protected]> * Bump log4j-api from 2.16.0 to 2.17.0 (#113) Bumps log4j-api from 2.16.0 to 2.17.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump log4j-core from 2.16.0 to 2.17.0 (#114) Bumps log4j-core from 2.16.0 to 2.17.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-core dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump up version from 1.1.7 to 1.1.8 (#115) Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Ravindranath Kakarla <[email protected]> Co-authored-by: Kexin Hui <[email protected]> Co-authored-by: Linyu Yao <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove extra text from Protobuf schema generation (#132) * Remove extra text from Protobuf schema generation Co-authored-by: Kexin Hui <[email protected]> * Add documentation for Protobuf support (#134) * Add documentation for Protobuf support Co-authored-by: Kexin Hui <[email protected]> * Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> * Protobuf support (#60) * Add message indices parser for Protobuf schemas. * Encode Protobuf messages with message index Co-authored-by: Ravindranath Kakarla <[email protected]> * Bump up version to v1.1.1 * Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> * Pull everit from maven central instead of jitpack (#54) Co-authored-by: mohit <[email protected]> * Fix resource clean up in Kafka integration test * Add message indices parser for Protobuf schemas. (#52) Co-authored-by: Ravindranath Kakarla <[email protected]> * Update dependencies for Protobuf and create class structure cr: https://code.amazon.com/reviews/CR-52122396 * Add ProtobufMessageType to aid Protobuf deserialization cr: https://code.amazon.com/reviews/CR-54151956 * Protobuf DynamicMessage deserialization cr: https://code.amazon.com/reviews/CR-54424261 * Fix serializer build issue * Fix serializer build issue * Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object (#72) Co-authored-by: Ravindranath Kakarla <[email protected]> * Add a utility to derive class name from message descriptor (#74) * Refactor GlueSchemaRegistryDataFormatDeserializer to accept Schema object * Add a utility to derive Protobuf compiled Java class name from FileDescriptor. Co-authored-by: Ravindranath Kakarla <[email protected]> * Add support to deserialize to Protobuf POJOs. (#76) Co-authored-by: Ravindranath Kakarla <[email protected]> * Handle ProtobufMessageType not set. (#85) Co-authored-by: Ravindranath Kakarla <[email protected]> * Introduce cache to improve Protobuf serializer/de-serializer performance. (#90) Co-authored-by: Ravindranath Kakarla <[email protected]> * Add integration tests for Protobuf support (#99) * Protobuf integration tests for Kinesis use-cases * Protobuf integration test cases for Kafka use-cases. Co-authored-by: Ravindranath Kakarla <[email protected]> * Upgrade to latest versions of Protobuf dependencies. * Protobuf support (#117) * Modify UserAgent to emit usage metrics (#77) * Add tests to include key and value schemas both (#79) Co-authored-by: Mohit Paliwal <[email protected]> * Bump to 1.1.3 (#78) * Modify UserAgent to emit usage metrics * Bump version to 1.1.3 Co-authored-by: Ravindranath Kakarla <[email protected]> * Update README.md * Upgrade Kafka version to 2.8.1 * Bump up version to 1.1.4 * Upgrade transitive dependencies to latest versions. Co-authored-by: Ravindranath Kakarla <[email protected]> * Remove configuration logging information (#89) * Bump up version to 1.1.5 (#91) * Specify the avro-maven-plugin version explicitly (#97) Co-authored-by: Ravindranath Kakarla <[email protected]> * Fix bug with canary tests running the entire suite. (#102) Co-authored-by: Ravindranath Kakarla <[email protected]> * Upgrade to latest versions of Protobuf dependencies. * Bump log4j-core from 2.13.2 to 2.15.0 (#107) * Bump log4j-api from 2.13.2 to 2.15.0 (#106) Bumps log4j-api from 2.13.2 to 2.15.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump up version from 1.1.5 to 1.1.6 (#108) Co-authored-by: Mohit Paliwal <[email protected]> * Bump log4j-api from 2.15.0 to 2.16.0 (#109) Bumps log4j-api from 2.15.0 to 2.16.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump up version from 1.1.6 to 1.1.7 (#111) Co-authored-by: Mohit Paliwal <[email protected]> * Pull current region for integration tests (#112) Co-authored-by: Mohit Paliwal <[email protected]> * Bump log4j-api from 2.16.0 to 2.17.0 (#113) Bumps log4j-api from 2.16.0 to 2.17.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-api dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump log4j-core from 2.16.0 to 2.17.0 (#114) Bumps log4j-core from 2.16.0 to 2.17.0. --- updated-dependencies: - dependency-name: org.apache.logging.log4j:log4j-core dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump up version from 1.1.7 to 1.1.8 (#115) Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Ravindranath Kakarla <[email protected]> Co-authored-by: Kexin Hui <[email protected]> Co-authored-by: Linyu Yao <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove extra text from Protobuf schema generation (#132) * Remove extra text from Protobuf schema generation Co-authored-by: Kexin Hui <[email protected]> * Add documentation for Protobuf support (#134) * Add documentation for Protobuf support Co-authored-by: Kexin Hui <[email protected]> Co-authored-by: Ravindranath Kakarla <[email protected]> Co-authored-by: mohit <[email protected]> Co-authored-by: Mohit Paliwal <[email protected]> Co-authored-by: Kexin Hui <[email protected]> Co-authored-by: Jake Baker <[email protected]> Co-authored-by: Mark Lambert <[email protected]> Co-authored-by: Thomas Vestergaard Trolle <[email protected]> Co-authored-by: Kexin <[email protected]> Co-authored-by: Linyu Yao <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes issue #63
Description of changes:
.gitignore
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.