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

Set compatibility config to NONE for protobuf subjects #48

Merged
merged 15 commits into from
Oct 3, 2024

Conversation

siddhant2001
Copy link
Contributor

No description provided.

@siddhant2001 siddhant2001 requested a review from a team as a code owner September 30, 2024 14:20
@ravisingal
Copy link
Contributor

@kishansairam9 is working on a generic solution.

@ravisingal ravisingal changed the title ENG-50955: Set compatibility config to NONE for protobuf subjects (WIP) Set compatibility config to NONE for protobuf subjects (WIP) Sep 30, 2024
@kishansairam9
Copy link
Contributor

@kishansairam9 is working on a generic solution.

Adding each proto is painful, there has been a meeting where laxman and team decided on having a cron job to set for all protobufs. No need to override each proto individually. I think this is the way to go forward, my old PR is abandoned

# update packages
RUN apt update && apt upgrade -y

RUN apt install -y curl && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need them?

# setup virtual environment
RUN python -m venv /opt/venv

# create directory and copy pinot-data-metrics directory
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix the comments

@@ -0,0 +1,6 @@
certifi==2024.8.30
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need all of them explicitly? should we omit transitive dependencies and let pip figure it out?

data:
application.conf: |
{
"defaultSchemaType": {{ .Values.compatibilityChanger.config.defaultSchemaType }}
Copy link
Contributor

Choose a reason for hiding this comment

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

schemaType -> compatibilityType

logging.basicConfig(level=logging.INFO, format='%(asctime)s - %(levelname)s - %(message)s')

# Read helm values
host = os.getenv('SCHEMA_REGISTRY_HOST', 'localhost')
Copy link
Contributor

Choose a reason for hiding this comment

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

make this configurable

for subject in tqdm(subjects):
versions = get_subject_versions(subject)
if not versions:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

log a warn when no version available.


def main():
subjects = get_subjects()
logging.info(f"All subjects: {subjects}")
Copy link
Contributor

Choose a reason for hiding this comment

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

debug log or just log number of subjects

response = requests.get(url)
return response.json()

def get_subject_version_details(subject, version):
Copy link
Contributor

Choose a reason for hiding this comment

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

get_subject_version_details -> get_schema_version


# Select the first version
first_version = versions[0]
details = get_subject_version_details(subject, first_version)
Copy link
Contributor

Choose a reason for hiding this comment

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

details -> schema

charset-normalizer==3.3.2
idna==3.10
requests==2.32.3
tqdm==4.66.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment from @ravisingal : Progress bar may not be required for cron.

.gitignore Outdated
.idea
*__pycache__*
*cc*
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks too greedy and i think this can omit necessary files as well.

@laxmanchekka laxmanchekka changed the title Set compatibility config to NONE for protobuf subjects (WIP) Set compatibility config to NONE for protobuf subjects Oct 1, 2024
laxmanchekka
laxmanchekka previously approved these changes Oct 1, 2024
@laxmanchekka
Copy link
Contributor

laxmanchekka commented Oct 1, 2024

@ravisingal : can you please check this once from helm, k8s and git actions perspective as well

helm/values.yaml Outdated Show resolved Hide resolved
hypertraceDocker {
defaultImage {
imageName.set("schema-compatibility")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

add dockerFile.set(file("Dockerfile"))


hypertraceDocker {
defaultImage {
imageName.set("schema-compatibility")
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this?

@siddhant2001 siddhant2001 merged commit ed3c772 into main Oct 3, 2024
3 checks passed
@siddhant2001 siddhant2001 deleted the compability-changer branch October 3, 2024 07:28
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.

4 participants