[improve][schema]Improve the getSchema() method in SchemaRegistryServiceImpl.java

[nikam14]

Motivation

In SchemaRegistryServiceImpl.java there are two getSchema() methods.
1 - getSchema(String schemaId)
2 - getSchema(String schemaId, SchemaVersion version)
In the first method after getting Schema it is checked that whether the Schema is deleted or not. In Second method it is not checked when Schema.version=Latest . Which means it may return a deleted Schema.
So, checking it in getSchema(String schemaId, SchemaVersion version) can improve code, prevent from any future issue.

Modifications

checking Schema is deleted or not before return in getSchema(String schemaId, SchemaVersion version) method.
If it is deleted then return Null else return Schema.
Removing the check from getSchema(String schemaId) and adding it in getSchema(String schemaId, SchemaVersion version).

Verifying this change

• Make sure that the change passes the CI checks.

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository

[BewareMyPower]

Please add a test to show the changes make sense

[nikam14]

changes are covered in SchemaServiceTest.java

[lhotari]

changes are covered in SchemaServiceTest.java

@nikam14 Please add a test that shows that this change was needed. I don't see any test changes in this PR, so that is missing.

[lhotari]

In SchemaRegistryServiceImpl.java there are two getSchema() methods.
1 - getSchema(String schemaId)
2 - getSchema(String schemaId, SchemaVersion version)
In the first method after getting Schema it is checked that whether the Schema is deleted or not. In Second method it is not checked when Schema.version=Latest .
Adding the check can improve code.

Instead of explaining how the code is implemented, please explain why it was a problem instead. You can show this is a test too.

[nikam14]

These changes not cover any issue.
Changes are covered by existing tests.
This PR just improves code.

[lhotari]

These changes not cover any issue. Changes are covered by existing tests. This PR just improves code.

@nikam14 That doesn't make much sense.
In the motivation there is "So, checking it in getSchema(String schemaId, SchemaVersion version) can improve code, prevent from any future issue.".
Please add a test case for the potential future issue.

[nikam14]

changes are covered in Test addLotsOfEntriesThenDelete()-SchemaServiceTest.java. No need to add test case.

[lhotari]

changes are covered in Test addLotsOfEntriesThenDelete()-SchemaServiceTest.java. No need to add test case.

please check my comment #23354 (comment) . What is the potential future issue you would like to prepare for? Could you please add a test case for that?