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

CCR: Rename follow-task parameters and stats #34836

Merged
merged 4 commits into from
Oct 25, 2018

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Oct 25, 2018

This commit renames the follow-task parameters and its stats.
Below is the list of the changes:

Params

  • remote_cluster (unchanged)
  • leader_index (unchanged)
  • max_batch_operation_count -> max_read_request_operation_count
  • max_batch_size -> max_read_request_size
  • max_write_request_operation_count (new)
  • max_write_request_size (new)
  • max_concurrent_read_batches -> max_outstanding_read_requests
  • max_concurrent_write_batches -> max_outstanding_write_requests
  • max_write_buffer_size (unchanged)
  • max_write_buffer_count (unchanged)
  • max_retry_delay (unchanged)
  • poll_timeout -> read_poll_timeout

Stats

  • remote_cluster (unchanged)
  • leader_index (unchanged)
  • follower_index (unchanged)
  • shard_id (unchanged)
  • leader_global_checkpoint (unchanged)
  • leader_max_seq_no (unchanged)
  • follower_global_checkpoint (unchanged)
  • follower_max_seq_no (unchanged)
  • last_requested_seq_no (unchanged)
  • number_of_concurrent_reads -> outstanding_read_requests
  • number_of_concurrent_writes -> outstanding_write_requests
  • buffer_size_in_bytes -> write_buffer_size_in_bytes (new)
  • number_of_queued_writes -> write_buffer_operation_count
  • mapping_version -> follower_mapping_version
  • total_fetch_time_millis -> total_read_time_millis
  • total_fetch_remote_time_millis -> total_read_remote_exec_time_millis
  • number_of_successful_fetches -> successful_read_requests
  • number_of_failed_fetches -> failed_read_requests
  • operation_received -> operations_read
  • total_transferred_bytes -> bytes_read
  • total_index_time_millis -> total_write_time_millis [?]
  • number_of_successful_bulk_operations -> successful_write_requests
  • number_of_failed_bulk_operations -> failed_write_requests
  • number_of_operations_indexed -> operations_written
  • fetch_exception -> read_exceptions
  • time_since_last_read_millis -> time_since_last_read_millis

Closes #34703

/cc @dliappis and @chrisronline

This commit renames the follow-task parameters and its stats.
Below are the changes:

## Params
- remote_cluster (unchanged)
- leader_index (unchanged)
- max_read_request_operation_count -> max_read_request_operation_count
- max_batch_size -> max_read_request_size
- max_write_request_operation_count (new)
- max_write_request_size (new)
- max_concurrent_read_batches -> max_outstanding_read_requests
- max_concurrent_write_batches -> max_outstanding_write_requests
- max_write_buffer_size (unchanged)
- max_write_buffer_count (unchanged)
- max_retry_delay (unchanged)
- poll_timeout -> read_poll_timeout

## Stats
- remote_cluster (unchanged)
- leader_index (unchanged)
- follower_index (unchanged)
- shard_id (unchanged)
- leader_global_checkpoint (unchanged)
- leader_max_seq_no (unchanged)
- follower_global_checkpoint (unchanged)
- follower_max_seq_no (unchanged)
- last_requested_seq_no (unchanged)
- number_of_concurrent_reads -> outstanding_read_requests
- number_of_concurrent_writes -> outstanding_write_requests
- buffer_size_in_bytes -> write_buffer_size_in_bytes (new)
- number_of_queued_writes -> write_buffer_operation_count
- mapping_version -> follower_mapping_version
- total_fetch_time_millis -> total_read_time_millis
- total_fetch_remote_time_millis -> total_read_remote_exec_time_millis
- number_of_successful_fetches -> successful_read_requests
- number_of_failed_fetches -> failed_read_requests
- operation_received -> operations_read
- total_transferred_bytes -> bytes_read
- total_index_time_millis -> total_write_time_millis [?]
- number_of_successful_bulk_operations -> successful_write_requests
- number_of_failed_bulk_operations -> failed_write_requests
- number_of_operations_indexed -> operations_written
- fetch_exception -> read_exceptions
- time_since_last_read_millis -> time_since_last_read_millis
@dnhatn dnhatn added >non-issue :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features labels Oct 25, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@jasontedor
Copy link
Member

@elasticmachine run gradle build tests

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

The only one that differs from what we discussed is total_index_time_millis -> total_write_time_millis. I think we should keep total_index_time_millis. I don't want to block this PR on another discussion though so I am going to defer to @bleskes to make a decision in the morning. So let's do this:

  • if we want to change back to total_index_time_millis, @martijnvg please make the change in the morning, @bleskes dismiss my review and approve the PR, and @martijnvg let us get it merged and backported
  • if we want to keep total_write_time_millis, @bleskes dismiss my review and approve the PR, and @martijnvg let us get it merged and backported
  • after this, we need to remove the linked issue as a blocker from the relevant places

That is, this PR LGTM save this one issue.

Thank you @dnhatn for all the work here, this was a big change and it looks thorough to me. I really appreciate that you made all the code changes to align the variable names with the API names. 🙏

@dliappis
Copy link
Contributor

Thanks for the heads up @dnhatn , this is immensely helpful for adjusting our benchmarking workflows.

@martijnvg
Copy link
Member

build oss-distro-docs please

@martijnvg
Copy link
Member

The only one that differs from what we discussed is total_index_time_millis -> total_write_time_millis. I think we should keep total_index_time_millis

Personally I think that total_write_time_millis is more consistent with the other shard follow stats field names and I'm okay with keeping it.

@bleskes
Copy link
Contributor

bleskes commented Oct 25, 2018

Personally I think that total_write_time_millis is more consistent with the other shard follow stats field names and I'm okay with keeping it.

I'm of the same mind.

@martijnvg do you mind pulling it in and back porting?

@dnhatn all the things that Jason and more. thanks!

@martijnvg
Copy link
Member

do you mind pulling it in and back porting?

I will pull it in and backport.

@martijnvg
Copy link
Member

martijnvg commented Oct 25, 2018

Note that the docs build was successful:

08:17:09 BUILD SUCCESSFUL in 3m 32s
08:17:09 275 actionable tasks: 274 executed, 1 up-to-date
08:17:09 
08:17:09 Deprecated Gradle features were used in this build, making it incompatible with Gradle 5.0.
08:17:09 Use '--warning-mode all' to show the individual deprecation warnings.
08:17:09 See https://docs.gradle.org/4.10/userguide/command_line_interface.html#sec:command_line_warnings
08:17:10 runbld>>> <<<<<<<<<<<< SCRIPT EXECUTION END <<<<<<<<<<<<
08:17:10 runbld>>> DURATION: 216536ms
08:17:10 runbld>>> STDOUT: 544135 bytes
08:17:10 runbld>>> STDERR: 5155 bytes
08:17:10 runbld>>> WRAPPED PROCESS: FAILURE (1)
08:17:10 runbld>>> Searching for build metadata in /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request+oss-distro-docs
08:17:12 runbld>>> Storing build metadata: 
08:17:12 runbld>>> Searching for junit test output files with the pattern: TEST-.*\.xml$ in: /var/lib/jenkins/workspace/elastic+elasticsearch+pull-request+oss-distro-docs
08:17:12 runbld>>> Found 4 test output files
08:17:13 runbld>>> Test output logs contained: Errors: 0 Failures: 0 Tests: 1309 Skipped: 159
08:17:13 runbld>>> FAILURES: 0
08:17:13 runbld>>> BUILD: https://c150076387b5421f9154dfbf536e5c60.us-west1.gcp.cloud.es.io:9243/build-1531848254847/t/20181025061331-1E1F8BEC
08:17:13 runbld>>> NO MAIL GENERATED
08:17:19 Build step 'Execute shell' marked build as failure

but runbld could not generate an email.

This is the actual failure:

> Task :docs:integTestCluster#stop
Task ':docs:integTestCluster#stop' is not up-to-date because:
* Try:
  Task has not declared any outputs despite executing actions.
Run with --stacktrace option to get the stack trace. Run with --debug option to get more log output. Run with --scan to get full insights.

Running ./gradlew --info -p docs -Dtests.distribution=oss-zip check locally on the branch passed. So I think it is ok to ignore this.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thank you Nhat! 💯

@martijnvg martijnvg merged commit ff49e79 into elastic:master Oct 25, 2018
martijnvg pushed a commit that referenced this pull request Oct 25, 2018
* CCR: Rename follow parameters and stats

This commit renames the follow-task parameters and its stats.
Below are the changes:

## Params
- remote_cluster (unchanged)
- leader_index (unchanged)
- max_read_request_operation_count -> max_read_request_operation_count
- max_batch_size -> max_read_request_size
- max_write_request_operation_count (new)
- max_write_request_size (new)
- max_concurrent_read_batches -> max_outstanding_read_requests
- max_concurrent_write_batches -> max_outstanding_write_requests
- max_write_buffer_size (unchanged)
- max_write_buffer_count (unchanged)
- max_retry_delay (unchanged)
- poll_timeout -> read_poll_timeout

## Stats
- remote_cluster (unchanged)
- leader_index (unchanged)
- follower_index (unchanged)
- shard_id (unchanged)
- leader_global_checkpoint (unchanged)
- leader_max_seq_no (unchanged)
- follower_global_checkpoint (unchanged)
- follower_max_seq_no (unchanged)
- last_requested_seq_no (unchanged)
- number_of_concurrent_reads -> outstanding_read_requests
- number_of_concurrent_writes -> outstanding_write_requests
- buffer_size_in_bytes -> write_buffer_size_in_bytes (new)
- number_of_queued_writes -> write_buffer_operation_count
- mapping_version -> follower_mapping_version
- total_fetch_time_millis -> total_read_time_millis
- total_fetch_remote_time_millis -> total_read_remote_exec_time_millis
- number_of_successful_fetches -> successful_read_requests
- number_of_failed_fetches -> failed_read_requests
- operation_received -> operations_read
- total_transferred_bytes -> bytes_read
- total_index_time_millis -> total_write_time_millis [?]
- number_of_successful_bulk_operations -> successful_write_requests
- number_of_failed_bulk_operations -> failed_write_requests
- number_of_operations_indexed -> operations_written
- fetch_exception -> read_exceptions
- time_since_last_read_millis -> time_since_last_read_millis

* add test for max_write_request_(operation_count|size)
@dnhatn
Copy link
Member Author

dnhatn commented Oct 25, 2018

Thanks everyone :)

@dnhatn dnhatn deleted the ccr-rename branch October 25, 2018 12:07
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 26, 2018
* master: (74 commits)
  XContent: Check for bad parsers (elastic#34561)
  Docs: Align prose with snippet (elastic#34839)
  document the search context is freed if the scroll is not extended (elastic#34739)
  Test: Lookup node versions on rest test start (elastic#34657)
  SQL: Return error with ORDER BY on non-grouped. (elastic#34855)
  Reduce channels in AbstractSimpleTransportTestCase (elastic#34863)
  [DOCS] Updates Elasticsearch monitoring tasks (elastic#34339)
  Check self references in metric agg after last doc collection (elastic#33593) (elastic#34001)
  [Docs] Add `indices.query.bool.max_clause_count` setting (elastic#34779)
  Add 6.6.0 version to master (elastic#34847)
  Test: ensure char[] doesn't being with prefix (elastic#34816)
  Remove static import from HLRC doc snippet (elastic#34834)
  Logging: server: clean up logging (elastic#34593)
  Logging: tests: clean up logging (elastic#34606)
  SQL: Fix edge case: `<field> IN (null)` (elastic#34802)
  [Test] Mute FullClusterRestartIT.testShrink() until test is fixed
  SQL: Introduce ODBC mode, similar to JDBC (elastic#34825)
  SQL: handle X-Pack or X-Pack SQL not being available in a more graceful way (elastic#34736)
  [Docs] Add explanation for code snippets line width (elastic#34796)
  CCR: Rename follow-task parameters and stats (elastic#34836)
  ...
@dliappis
Copy link
Contributor

Many thanks @dnhatn for the description of the changes; one question:

I guess

image
in your description actually refers to the change:

max_batch_operation_count -> max_read_request_operation_count

right?

@dnhatn
Copy link
Member Author

dnhatn commented Oct 28, 2018

@dliappis You're correct. I've updated the description.

kcm pushed a commit that referenced this pull request Oct 30, 2018
* CCR: Rename follow parameters and stats

This commit renames the follow-task parameters and its stats.
Below are the changes:

## Params
- remote_cluster (unchanged)
- leader_index (unchanged)
- max_read_request_operation_count -> max_read_request_operation_count
- max_batch_size -> max_read_request_size
- max_write_request_operation_count (new)
- max_write_request_size (new)
- max_concurrent_read_batches -> max_outstanding_read_requests
- max_concurrent_write_batches -> max_outstanding_write_requests
- max_write_buffer_size (unchanged)
- max_write_buffer_count (unchanged)
- max_retry_delay (unchanged)
- poll_timeout -> read_poll_timeout

## Stats
- remote_cluster (unchanged)
- leader_index (unchanged)
- follower_index (unchanged)
- shard_id (unchanged)
- leader_global_checkpoint (unchanged)
- leader_max_seq_no (unchanged)
- follower_global_checkpoint (unchanged)
- follower_max_seq_no (unchanged)
- last_requested_seq_no (unchanged)
- number_of_concurrent_reads -> outstanding_read_requests
- number_of_concurrent_writes -> outstanding_write_requests
- buffer_size_in_bytes -> write_buffer_size_in_bytes (new)
- number_of_queued_writes -> write_buffer_operation_count
- mapping_version -> follower_mapping_version
- total_fetch_time_millis -> total_read_time_millis
- total_fetch_remote_time_millis -> total_read_remote_exec_time_millis
- number_of_successful_fetches -> successful_read_requests
- number_of_failed_fetches -> failed_read_requests
- operation_received -> operations_read
- total_transferred_bytes -> bytes_read
- total_index_time_millis -> total_write_time_millis [?]
- number_of_successful_bulk_operations -> successful_write_requests
- number_of_failed_bulk_operations -> failed_write_requests
- number_of_operations_indexed -> operations_written
- fetch_exception -> read_exceptions
- time_since_last_read_millis -> time_since_last_read_millis

* add test for max_write_request_(operation_count|size)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/CCR Issues around the Cross Cluster State Replication features >non-issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants