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

Add backlogSize in topicStats #5914

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Add backlogSize in topicStats #5914

merged 1 commit into from
Jan 7, 2020

Conversation

k2la
Copy link
Contributor

@k2la k2la commented Dec 23, 2019

Fix storageSize and add backlogSize in topicStats

Modifications

@k2la
Copy link
Contributor Author

k2la commented Dec 24, 2019

retest this please

@codelipenghui
Copy link
Contributor

retest this please

@massakam
Copy link
Contributor

rerun integration tests
rerun java8 tests

@massakam
Copy link
Contributor

rerun integration tests

2 similar comments
@k2la
Copy link
Contributor Author

k2la commented Dec 28, 2019

rerun integration tests

@codelipenghui
Copy link
Contributor

rerun integration tests

@sijie
Copy link
Member

sijie commented Jan 1, 2020

run integration tests

@k2la
Copy link
Contributor Author

k2la commented Jan 5, 2020

rerun integration tests

1 similar comment
@codelipenghui
Copy link
Contributor

rerun integration tests

@nkurihar nkurihar merged commit d1d5cf7 into apache:master Jan 7, 2020
@nkurihar nkurihar added this to the 2.5.1 milestone Jan 7, 2020
@k2la k2la deleted the add_backlogSize_in_topicStat branch January 7, 2020 01:38
@sijie sijie modified the milestones: 2.5.1, 2.6.0 Jan 22, 2020
tuteng pushed a commit to AmateurEvents/pulsar that referenced this pull request Feb 23, 2020
tuteng pushed a commit to AmateurEvents/pulsar that referenced this pull request Mar 21, 2020
tuteng pushed a commit that referenced this pull request Apr 13, 2020
jiazhai pushed a commit to jiazhai/pulsar that referenced this pull request May 18, 2020
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@rdhabalia
Copy link
Contributor

this is not a good practice to break the metrics compatibility. After upgrading the release, it broke our users monitoring and it's not acceptable. we can always add new metrics if needed but it's better to avoid changing existing behavior as there are monitoring and alerts setup of existing metrics.
I would like to revert this change. Please let me know if anyone has any concerns?

@k2la
Copy link
Contributor Author

k2la commented May 20, 2021

@rdhabalia
In my modification, I modified "storageSize" and "backlogSize" based on #5108.
I think if you revert mine, you need to fix #5108 as well.

Also, mine is included in 2.5.1 or later.
the compatibility will be broken by reverting it, is that ok?

@rdhabalia
Copy link
Contributor

the compatibility will be broken by reverting it, is that ok?

Compatibility was broken in 2.5 and it can be fixed in later version.

@codelipenghui
Copy link
Contributor

@rdhabalia If revert this change, we will break the behavior for version 2.5.x - 2.7.x?

@eolivelli
Copy link
Contributor

@rdhabalia probably it is too late.
We should keep now compatibility with at least 2.6.x and 2.7.x.

@rdhabalia
Copy link
Contributor

it has already broken for all the systems which have been using Pulsar for last 7 years. if any system was using the metrics before 2.5, definitely needs this compatibility back and the one which is not using it, for it this doesn't matter. Introducing incompatibility and not handling is not a good practice. so, it's never late to make things right.

@codelipenghui
Copy link
Contributor

@rdhabalia Could you please start an email thread for discussion? As far as I understand, most of the users are using 2.5.x 2.6.x 2.7.x. If there are 80% users working on 2.5.x - 2.7.x, I don't think revert this change is a good choice, it will break again.

@eolivelli
Copy link
Contributor

So the problem is for people upgrading from 2.4.

I agree that we must not change the meaning of metrics and APIs.

Probably it is better to not do now the mistake we did in the past with this PR.

Users coming from 2.4 will have to update their monitoring systems.
I don't know how many users are impacted by this change nowadays

@rdhabalia
Copy link
Contributor

Users coming from 2.4 will have to update their monitoring systems. I don't know how many users are impacted by this change nowadays

I don't agree and it's not a responsible statement to say that it's fine to let old users suffer from the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants