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.

Sign up for GitHub

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

Jump to bottom

Decrease excessive visibility from pub throughout the crate #660

Open
wprzytula opened this issue Mar 15, 2023 · 1 comment
Open

Decrease excessive visibility from pub throughout the crate #660

wprzytula opened this issue Mar 15, 2023 · 1 comment
Assignees
@wprzytula
Labels
API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API
Milestone
1.0.0

Comments

@wprzytula
Copy link
Collaborator

Our codebase suffers from the prevalent light-hearted use of the pub qualifier. In order to reduce chances of having to incur backward-incompatible changes in the future, we should tackle with this problem once and (hopefully) forever.
This should definitely be done before we release 1.0.

Some suggestions have already been issued (e.g. #497).
@piodul piodul added this to the 0.9.0 milestone Mar 24, 2023
@piodul piodul added the API-breaking This might introduce incompatible API changes label Mar 28, 2023
@wprzytula wprzytula self-assigned this Apr 12, 2023
@piodul piodul mentioned this issue Apr 19, 2023
Closed
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 19, 2023
@avelanarius
routing: remove Node struct
fb90490 
Remove struct scylla::routing::Node from routing.rs. This structure
was introduced in 2020 in 1c8df89
and it is unused in our current code (and it looks like the original
commit didn't even use it at all). 

Moreover, this struct clashes with scylla::transport::Node making it
error-prone to find the wrong Node struct for a user of the driver. 
The deleted Node structure also relied solely on SocketAddr, while we 
want to identify the nodes by host_id (as in scylla::transport::Node).

Refs scylladb#660.
This was referenced Jul 19, 2023
Merged
Merged
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 19, 2023
@avelanarius
connection: remove re-export of QueryResult
6a4c637 
Remove re-export of QueryResult (from scylla::transport::query_result::QueryResult
to scylla::transport::connection::QueryResult). The motivation for this
change is to reduce the number of re-exports in the codebase (some of
them don't make sense and are there for legacy reasons), as a part
of our effort of stabilizing the API.

It probably makes sense to follow this up with moving QueryResult
from transport/ module to a more appropriate module (query/ module?).

Refs scylladb#660
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 19, 2023
@avelanarius
frame: reduce visibility of several funcs/consts
df09d3d 
Reduce visibility of several functions and constants which are only
used internally within frame and don't have to be exposed publicly.
In this patch only the most obvious ones are tackled (ones that require
a change solely in scylla-cql). This change is a part of our effort
to stabilize the API.

Refs scylladb#660
@avelanarius avelanarius mentioned this issue Jul 19, 2023
Merged
8 tasks
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 21, 2023
@avelanarius
batch: remove re-export of consistency enums
8b8c467 
Remove re-export of consistency enums in batch.rs:
- scylla:statement::batch::Consistency
- scylla:statement::batch::SerialConsistency

A user of the driver should instead use:
- scylla:statement::Consistency
- scylla:statement::SerialConsistency

This change is a part of our effort to stabilize the API and reduce
the number of pub exports.

Refs scylladb#660
@avelanarius avelanarius mentioned this issue Jul 21, 2023
Merged
8 tasks
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 21, 2023
@avelanarius
session: remove estimate_replicas_for_query
5ff6ee2 
Remove estimate_replicas_for_query from Session. A user of this function
can switch to using get_token_endpoints() from ClusterData directly.

It did not make sense for estimate_replicas_for_query() to be in
Session, as this is too much low-level function for Session and doesn't
fit the abstraction of Session.

As a followup, it might make sense to look into whether 
get_token_endpoints() from ClusterData should get removed as well:
ReplicaLocator can be used directly.

This change is a part of our effort to stabilize the API and reduce
the number of pub functions.

Refs scylladb#660
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 21, 2023
@avelanarius
session: remove get_tracing_info_custom
dd88b4b 
Remove Session::get_tracing_info_custom - a variant of
Session::get_tracing_info that allows for additional customization of
the fetch process (number of attempts, interval between attempts,
consistency level of fetching tracing info). This customization is now
moved to SessionConfig (as new parameters) and new helper functions
were introduced to SessionBuilder.

Since now this customization is on SessionConfig,
create_new_session_builder() in test_utils.rs returns a SessionBuilder
with pre-configured large number of TracingInfo fetch attempts, longer
delays. Previously each tracing test had to configure it on its own by
using get_tracing_info_custom.

This change is a part of our effort to stabilize the API and reduce
the number of pub functions.

Refs scylladb#660
@avelanarius avelanarius mentioned this issue Jul 21, 2023
Merged
8 tasks
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 21, 2023
@avelanarius
session: remove get_tracing_info_custom
4b19f74 
Remove Session::get_tracing_info_custom - a variant of
Session::get_tracing_info that allows for additional customization of
the fetch process (number of attempts, interval between attempts,
consistency level of fetching tracing info). This customization is now
moved to SessionConfig (as new parameters) and new helper functions
were introduced to SessionBuilder.

Since now this customization is on SessionConfig,
create_new_session_builder() in test_utils.rs returns a SessionBuilder
with pre-configured large number of TracingInfo fetch attempts, longer
delays (to account for a slow CI). Previously each tracing test had to
configure it on its own by using get_tracing_info_custom.

This change is a part of our effort to stabilize the API and reduce
the number of pub functions.

Refs scylladb#660
avelanarius added a commit to avelanarius/scylla-rust-driver that referenced this issue Jul 27, 2023
@avelanarius
session: remove get_tracing_info_custom
ebfc00d 
Remove Session::get_tracing_info_custom - a variant of
Session::get_tracing_info that allows for additional customization of
the fetch process (number of attempts, interval between attempts,
consistency level of fetching tracing info). This customization is now
moved to SessionConfig (as new parameters) and new helper functions
were introduced to SessionBuilder.

Since now this customization is on SessionConfig,
create_new_session_builder() in test_utils.rs returns a SessionBuilder
with pre-configured large number of TracingInfo fetch attempts, longer
delays (to account for a slow CI). Previously each tracing test had to
configure it on its own by using get_tracing_info_custom.

This change is a part of our effort to stabilize the API and reduce
the number of pub functions.

Refs scylladb#660
This was referenced Jul 28, 2023
Merged
Merged
Merged
@wprzytula wprzytula added the API-stability Part of the effort to stabilize the API label Jul 30, 2023
@wprzytula wprzytula mentioned this issue Jul 31, 2023
Merged
6 tasks
@wprzytula
Copy link
Collaborator Author

Most work regarding this issue has already been done. Keeping this open, however, to remember us about revisiting the whole API before we release 1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wprzytula wprzytula

Labels
API-breaking This might introduce incompatible API changes API-stability Part of the effort to stabilize the API
Projects
None yet
Milestone
1.0.0
Development

No branches or pull requests
2 participants
@piodul @wprzytula