[Non-Inclusive Language] Replace master in comments #1778

manasvinibs · 2022-06-22T18:31:19Z

Signed-off-by: manasvinibs [email protected]

Description

As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories.

In this change, we are replacing 'master' in comments with 'cluster_manager'.

Issues Resolved

#1692

Check List

joshuarrrr · 2022-06-23T01:17:34Z

src/plugins/telemetry/server/telemetry_collection/get_local_license.ts

 async function getLicenseFromLocalOrMaster(opensearchClient: OpenSearchClient) {
-  // Fetching the local license is cheaper than getting it from the master node and good enough
+  // Fetching the local license is cheaper than getting it from the cluster_manager node and good enough


It's weird for me to see the comments being updated before the function names. I'm guessing that's planned for another PR, but I'd prefer if they were all together so we could validate that the comments still match the code.

It's weird for me to see the comments being updated before the function names. I'm guessing that's planned for another PR, but I'd prefer if they were all together so we could validate that the comments still match the code.

I looked into other open issues related to this task and I don't see any specific issue for updating the function names as such. I think I should have that updated as part of this PR for better readability. Thanks for catching this!

kavilla · 2022-06-23T08:26:06Z

src/plugins/telemetry/server/telemetry_collection/get_local_license.ts

-async function getLicenseFromLocalOrMaster(opensearchClient: OpenSearchClient) {
-  // Fetching the local license is cheaper than getting it from the master node and good enough
+async function getLicenseFromLocalOrClusterManager(opensearchClient: OpenSearchClient) {
+  // Fetching the local license is cheaper than getting it from the cluster_manager node and good enough


NIT: I think the comments would be main or manager node? But if not, it should definitely be cluster manager node.

Also, can you do me a favor and mark this as deprecated and create an issue to verify if this is in use? We shouldn't have any licensing checks. It's all telemetry.

NIT: I think the comments would be main or manager node? But if not, it should definitely be cluster manager node.
Sure! Updated.
Also, can you do me a favor and mark this as deprecated and create an issue to verify if this is in use? We shouldn't have any licensing checks. It's all telemetry.
Created: #1787

joshuarrrr

I think we could improve a couple of these comments, but no blockers.

joshuarrrr · 2022-06-23T17:58:45Z

src/core/server/http/http_service.ts

@@ -164,7 +164,7 @@ export class HttpService
   * Indicates if http server has configured to start listening on a configured port.
   * We shouldn't start http service in two cases:
   * 1. If `server.autoListen` is explicitly set to `false`.
-   * 2. When the process is run as dev cluster master in which case cluster manager
+   * 2. When the process is run as dev cluster manager in which case cluster manager


nit - "in which case cluster manager" doesn't really make sense to me or add anything useful, so I'd recommend deleting.

Suggested change

* 2. When the process is run as dev cluster manager in which case cluster manager

* 2. When the process is run as dev cluster manager

joshuarrrr · 2022-06-23T18:04:20Z

src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/series/date_histogram.js

-    // master
+    // cluster_manager


I have no idea what the purpose of this comment is 😂 - I'd recommend removing as unhelpful

kavilla · 2022-06-23T21:04:14Z

@manasvinibs,

Does signing off with your full name works on main? For 2.x manasvinibs fails the DCO check because I believe it is looking at your GitHub name and see you use your full name.

manasvinibs · 2022-06-23T21:19:43Z

@manasvinibs,

Does signing off with your full name works on main? For 2.x manasvinibs fails the DCO check because I believe it is looking at your GitHub name and see you use your full name.

I just updated my GPG keys with user name matching my full name and re-signed commit with my full name. I hope this gets 'verified' now on 2.x.

codecov-commenter · 2022-06-23T22:20:04Z

Codecov Report

Merging #1778 (31fd295) into main (7c8eee4) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1778      +/-   ##
==========================================
- Coverage   67.51%   67.50%   -0.01%     
==========================================
  Files        3073     3073              
  Lines       59069    59069              
  Branches     8963     8963              
==========================================
- Hits        39881    39877       -4     
- Misses      17006    17008       +2     
- Partials     2182     2184       +2

Impacted Files	Coverage Δ
src/core/server/http/http_service.ts	`90.74% <ø> (ø)`
...nsearch/version_check/ensure_opensearch_version.ts	`86.66% <ø> (ø)`
...s_data/request_processors/series/date_histogram.js	`100.00% <ø> (ø)`
...ic/application/models/sense_editor/sense_editor.ts	`64.00% <0.00%> (-1.78%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c8eee4...31fd295. Read the comment docs.

…nction names As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories. Issue - opensearch-project#1692 Signed-off-by: Manasvini B Suryanarayana <[email protected]>

…nction names (#1778) As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories. Issue - #1692 Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit e32a259)

…nction names (#1778) (#1793) As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories. Issue - #1692 Signed-off-by: Manasvini B Suryanarayana <[email protected]> (cherry picked from commit e32a259) Co-authored-by: Manasvini B Suryanarayana <[email protected]>

manasvinibs requested a review from a team as a code owner June 22, 2022 18:31

manasvinibs force-pushed the Issue-1692 branch from 7874bb7 to f8b9648 Compare June 22, 2022 20:05

manasvinibs self-assigned this Jun 22, 2022

manasvinibs force-pushed the Issue-1692 branch from f8b9648 to 6c76407 Compare June 23, 2022 00:08

joshuarrrr reviewed Jun 23, 2022

View reviewed changes

manasvinibs force-pushed the Issue-1692 branch 2 times, most recently from 15c4e5d to 1ccb8e9 Compare June 23, 2022 01:37

manasvinibs requested a review from joshuarrrr June 23, 2022 01:38

kavilla reviewed Jun 23, 2022

View reviewed changes

joshuarrrr added the v2.1.0 label Jun 23, 2022

joshuarrrr previously approved these changes Jun 23, 2022

View reviewed changes

manasvinibs mentioned this pull request Jun 23, 2022

[Investigate] Verify if the function getLicenseFromLocalOrClusterManager is in use or not #1787

Open

manasvinibs dismissed joshuarrrr’s stale review via 2e6b950 June 23, 2022 20:27

manasvinibs force-pushed the Issue-1692 branch from 1ccb8e9 to 2e6b950 Compare June 23, 2022 20:27

manasvinibs requested review from kavilla and joshuarrrr June 23, 2022 20:29

joshuarrrr previously approved these changes Jun 23, 2022

View reviewed changes

kavilla added the deprecation label Jun 23, 2022

kavilla linked an issue Jun 23, 2022 that may be closed by this pull request

[Non-Inclusive Language] Replace master in comments and variables #1692

Closed

manasvinibs dismissed joshuarrrr’s stale review via 485d347 June 23, 2022 21:16

manasvinibs force-pushed the Issue-1692 branch from 2e6b950 to 485d347 Compare June 23, 2022 21:16

manasvinibs requested a review from joshuarrrr June 23, 2022 21:20

kavilla approved these changes Jun 23, 2022

View reviewed changes

kavilla added the backport 2.x label Jun 23, 2022

manasvinibs force-pushed the Issue-1692 branch from 31fd295 to ee9759a Compare June 23, 2022 23:59

joshuarrrr approved these changes Jun 24, 2022

View reviewed changes

joshuarrrr merged commit e32a259 into opensearch-project:main Jun 24, 2022

opensearch-trigger-bot bot mentioned this pull request Jun 24, 2022

[Backport 2.x] [Non-Inclusive Language] Replace master in comments #1793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Non-Inclusive Language] Replace master in comments #1778

[Non-Inclusive Language] Replace master in comments #1778

manasvinibs commented Jun 22, 2022

joshuarrrr Jun 23, 2022

manasvinibs Jun 23, 2022

kavilla Jun 23, 2022

manasvinibs Jun 23, 2022

joshuarrrr left a comment •

edited

Loading

joshuarrrr Jun 23, 2022

manasvinibs Jun 23, 2022

joshuarrrr Jun 23, 2022

kavilla commented Jun 23, 2022

manasvinibs commented Jun 23, 2022

codecov-commenter commented Jun 23, 2022

	* 2. When the process is run as dev cluster manager in which case cluster manager
	* 2. When the process is run as dev cluster manager

[Non-Inclusive Language] Replace master in comments #1778

[Non-Inclusive Language] Replace master in comments #1778

Conversation

manasvinibs commented Jun 22, 2022

Description

Issues Resolved

Check List

joshuarrrr Jun 23, 2022

Choose a reason for hiding this comment

manasvinibs Jun 23, 2022

Choose a reason for hiding this comment

kavilla Jun 23, 2022

Choose a reason for hiding this comment

manasvinibs Jun 23, 2022

Choose a reason for hiding this comment

joshuarrrr left a comment • edited Loading

Choose a reason for hiding this comment

joshuarrrr Jun 23, 2022

Choose a reason for hiding this comment

manasvinibs Jun 23, 2022

Choose a reason for hiding this comment

joshuarrrr Jun 23, 2022

Choose a reason for hiding this comment

kavilla commented Jun 23, 2022

manasvinibs commented Jun 23, 2022

codecov-commenter commented Jun 23, 2022

Codecov Report

joshuarrrr left a comment •

edited

Loading